[SILOptimizer] Add RedundantMoveValueElimination. #64190
Conversation
|
@swift-ci please test |
|
@swift-ci please benchmark |
lib/SIL/Utils/OwnershipUtils.cpp
Outdated
| if (auto borrowedValue = BorrowedValue(value)) { | ||
| return hasPointerEscape(borrowedValue); | ||
| } | ||
| for (auto use : value->getUses()) { |
There was a problem hiding this comment.
Checking immediate uses only makes sense for an owned value. Do you want an assert here and document that \p value must introduce an OSSA lifetime (either BorrowedValue or owned value)
There was a problem hiding this comment.
Fixed. And added a check in RedundantMoveValueElimination that the original value is owned.
nate-chandler
force-pushed
the
redundant-move-elim
branch
from
02cff7e to
7b54b32
Compare
|
@swift-ci please test |
|
@swift-ci please test macOS platform |
lib/SIL/Utils/OwnershipUtils.cpp
Outdated
| return hasPointerEscape(borrowedValue); | ||
| } | ||
| assert(value->getOwnershipKind() == OwnershipKind::Owned); | ||
| for (auto use : value->getUses()) { |
There was a problem hiding this comment.
What if there's an escape in the "extended" lifetime ? i.e a phi values escapes
There was a problem hiding this comment.
Thank you, great point. From recent discussion with @atrick, my understanding was that a ForwardingConsumes shouldn't be considered part of the same lifetime for the purposes of pointer escapes. If that applies even to branches, since a br(%owned) is a forwarding consume of %owned, it wouldn't be included. On the other hand, excluding phi-extended lifetime is inconsistent with the intuition that "things should be the same" whether (1) there's a merge and a phi or (2) the function body portion is cloned for each incoming value. So it's seeming to me like we do need to look through phis like you say.
If we need to look through phis, phis also probably need to be rejected as inputs to this function like they are rejected by If we need to look through phis, this function need to either reject them (like hasPointerEscape(BorrowedValue). It seems we also might not want to count them as "owned value introducers" (or allow them to be used in @atrick's forthcoming OwnedValue abstraction).hasPointerEscape(BorrowedValue)) or visit the transitive incoming values, which hasPointerEscape(BorrowedValue) could be doing as well.
I updated the patch to look through phis and ban them as input and accept them as input by adding transitive incoming values to the worklist.
nate-chandler
force-pushed
the
redundant-move-elim
branch
5 times, most recently
from
e6604bd to
62546a7
Compare
There is a preexisting function with this name that takes a BorrowedValue. The new function calls that preexisting function if a BorrowedValue can be constructed from the SILValue. Otherwise, it looks for direct uses of the value which qualify as "pointer escapes".
Adds to SemanticARCOpts a new step of removing move_value instructions if they are redundant. A move_value is redundant if it adds no new information or optimization opportunities. An example of adding information: a lifetime becoming lexical. The new lifetime's destroys cannot be hoisted over deinit barriers. An example of adding an optimization opportunity: the original value escapes but the value produced by the move_value does not escape. So destroys of the new value can be hoisted more aggressively.
nate-chandler
force-pushed
the
redundant-move-elim
branch
from
62546a7 to
5f561ed
Compare
|
@swift-ci please test |
|
@swift-ci please clean test linux platform |
1 similar comment
|
@swift-ci please clean test linux platform |
|
@swift-ci please smoke test linux platform |
|
@swift-ci please test linux platform |
Adds to
SemanticARCOptsa new step of removingmove_valueinstructions if they are redundant. Amove_valueis redundant if it adds no new information or optimization opportunities.An example of adding information: a lifetime becoming lexical. The new lifetime's destroys cannot be hoisted over deinit barriers.
An example of adding an optimization opportunity: the original value escapes but the value produced by the
move_valuedoes not escape. So destroys of the new value can be hoisted more aggressively.