Skip to content

[MoveOnlyAddressChecker] Maximize lifetimes. #66585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jun 17, 2023

Conversation

nate-chandler
Copy link
Contributor

Previously, the checker inserted destroys after each last use. Here, extend the lifetimes of fields as far as possible within their original (unchecked) limits.

rdar://99681073

@nate-chandler nate-chandler requested a review from atrick June 13, 2023 01:24
Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Just a few areas of confusion...

@nate-chandler nate-chandler force-pushed the rdar99681073 branch 2 times, most recently from a0d200a to 7fd9607 Compare June 14, 2023 17:49
@atrick atrick self-requested a review June 14, 2023 18:15
Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@nate-chandler nate-chandler force-pushed the rdar99681073 branch 3 times, most recently from 7ae4655 to 6c2ba68 Compare June 16, 2023 00:41
Vars of such types should be given lexical `alloc_stack`s by
`AllocBoxToStack` which requires that the `alloc_box` insts formed for
them have an associated borrow scope which in turn requires that type
lowering for move only structs and enums have their lexical bits set.

rdar://110901430
According to language rules, such lifetimes are fixed and the relative
order of their deinits is guaranteed.

rdar://110913116
The members were declared but undefined.
Its storage vector is intended to be of some type like
`std::vector<std::pair<Key, Optional<Value>>>`, i.e., some collection of
pairs whose `second` is an `Optional<Value>`.  So when constructing a
default instance of that pair, just construct an Optional in the None
case.
@nate-chandler nate-chandler marked this pull request as ready for review June 17, 2023 01:38
FieldSensitivePrunedLiveness is used as a vectorization of
PrunedLiveness.  An instance of FSPL with N elements needs to be able to
represent the same states as N instances of PL.

Previously, it failed to do that in two significant ways:

(1) It attempted to save space for which elements were live by using
    a range.  This failed to account for instructions which are users of
    non-contiguous fields of an aggregate.

    apply(
      @owned (struct_element_addr %s, #S.f1),
      @owned (struct_element_addr %s, #S.f3)
    )

(2) It used a single bit to represent whether the instruction was
    consuming.  This failed to account for instructions which consumed
    some fields and borrowed others.

    apply(
      @owned (struct_element_addr %s, #S.f1),
      @guaranteed (struct_element_addr %s, #S.f2)
    )

The fix for (1) is to use a bit vector to represent which elements
are used by the instruction.  The fix for (2) is to use a second bit
vector to represent which elements are _consumed_ by the instruction.

Adapted the move-checker to use the new representation.

rdar://110909290
Dumped more info and called llvm_unreachable on bad state.
Previously, the checker inserted destroys after each last use.  Here,
extend the lifetimes of fields as far as possible within their original
(unchecked) limits.

rdar://99681073
It's always the first line of the function, so try to do better.
Passing

```
-Xllvm -move-only-address-checker-disable-lifetime-extension=true
```

will skip the maximization of unconsumed field lifetimes.
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I reviewed for CCC.

@nate-chandler nate-chandler merged commit d312589 into swiftlang:main Jun 17, 2023
@nate-chandler nate-chandler deleted the rdar99681073 branch June 17, 2023 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants