Enable the CopyPropagation pass #35047
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gottesmm
reviewed
Dec 11, 2020
| /// unnacceptable because it fails to canonicalize common cases. | ||
| /// | ||
| /// TODO: If all client passes can maintain block numbers, then the | ||
| /// SmallDenseMaps/SetVectors can be replaced with bitsets/sparsesets. |
There was a problem hiding this comment.
Can you put an extra line here.
| /// def. For opaque values, we don't expect to see casts. | ||
| inline SILValue getCanonicalCopiedDef(SILValue v) { | ||
| while (true) { | ||
| if (auto *copy = dyn_cast<CopyValueInst>(v)) { |
There was a problem hiding this comment.
I have noticed that we are able to do this as follows. Looks much cleaner:
while (auto *copy = dyn_cast<CopyValueInst>(v)) {
v = copy->getOperand();
}
return v;
| } | ||
|
|
||
| // Return true if this instruction is marked as a final consume point of the | ||
| // current def's live range. A consuming instruction can only be claimed once |
| /// | ||
| /// For non-owned values, this remains empty. | ||
| class CanonicalOSSAConsumeInfo { | ||
| // Map blocks on the lifetime boundary to the last consuming instruction. |
| continue; // ignore operands that don't actually use the value | ||
| } | ||
| switch (operandOwnership.getValue()) { | ||
| case OperandOwnership::None: |
There was a problem hiding this comment.
This is more of a meta note: we should add OperandOwnership to SIL.rst I think. Once my other doc patch lands.
| // indicating whether they must end the lifetime. A user is interesting if it | ||
| // may be on the liveness boundary. Lifetime-ending users are always on the | ||
| // boundary so are always interesting. Non-lifetime-ending within a LiveWithin | ||
| // block are interesting because they may be the last use in the |
There was a problem hiding this comment.
Can you add a note in the comment at line 86 that says that "interesting" is defined on the documentation on this field. That way it is easy to find and is easy for the reader.
| /// Note: unlike OwnershipLiveRange, this represents a lifetime in terms of the | ||
| /// CFG boundary rather that the use set, and, because it is "pruned", it only | ||
| /// includes liveness generated by select uses. For example, it may not include | ||
| /// liveness up to destroy_value or end_borrow instructions. |
There was a problem hiding this comment.
You can get this effect with OwnershipLiveRange. We just give all of the uses.
That being said, I think your example here should be more explicit. My suggestion:
For example, liveness will not be generated (i.e. "pruned") for any block with only a single destroy_value or end_borrow use without other non-lifetime ending uses.
| /// TODO: Cleanup the resulting SIL. Delete instructions with no side effects | ||
| /// that produce values which are immediately destroyed after copy propagation. | ||
| /// TODO: Cleanup the resulting SIL by deleting instructions that produce dead | ||
| /// values (after removing its copies). |
| case OperandOwnership::Borrow: | ||
| // An entire borrow scope is considered a single use that occurs at the | ||
| // point of the end_borrow. | ||
| BorrowingOperand(use).visitLocalEndScopeUses([&lifetime](Operand *end){ |
There was a problem hiding this comment.
I am glad that BorrowingOperand was useful for you here!
A NonUse operand does not use the value itself, so it ignores ownership and does not require liveness. This is for operands that represent dependence on a type but are not actually passed the value of that type (e.g. they may refer an open_existential). This could be used for other dependence-only operands in the future. A TrivialUse operand has undefined ownership semantics, so it only handles values with ownership None. It does still require liveness. The name makes defining ownership for trivial operations perfectly obvious, increasing readability of OperandOwnership.cpp.
Canonicalizing OSSA provably minimizes the number of retains and releases within the boundaries of that lifetime. This eliminates the need for ad-hoc optimization of OSSA copies. This initial implementation only canonicalizes owned values, but canonicalizing guaranteed values is a simple extension. This was originally part of the CopyPropagation prototype years ago. Now OSSA is specified completely enough that it can be turned into a simple utility instead. CanonicalOSSALifetime uses PrunedLiveness to find the extended live range and identify the consumes on the boundary. All other consumes need their own copy. No other copies are needed. By running this after other transformations that affect OSSA lifetimes, we can avoid the need to run pattern-matching optimization to SemanticARC to recover from suboptimal patterns, which is not robust, maintainable, or efficient.
For now simply run the pass before SemanticARCOpts. This will probably be called as a utility from within SemanticARCOpts so it can be iteratively applied after other ARC-related transformations.
atrick
changed the title
|
Review feedback merged here: #35241 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For now simply run the pass before SemanticARCOpts. This will probably
be called as a utility from within SemanticARCOpts so it can be
iteratively applied after other ARC-related transformations.