[NFC] Add SSADestroyHoisting prototype (originally in CopyForwarding) #40392
Conversation
|
@nate-chandler I'm posting this hastily as an example to guide review of ShrinkBorrowScopes. I haven't taken the time to write unit tests so I'm sure I missed something. |
|
|
||
| // MARK: Analysis of the address SSA uses | ||
|
|
||
| bool hasPointerEscape = false; |
There was a problem hiding this comment.
Using member variables in this way is as bad as using global variables.
Functions, like findKnownUses, should return their results (or return via a reference parameter) and get their inputs as parameters. Otherwise it's a nightmare to figure out the flow of information in the algorithm.
I know that this is (or was) common practice, but we need to change this habit to make our code more maintainable.
There was a problem hiding this comment.
@eeckstein I think the code is very clear that these members are the result of the SSA-use analysis. hasPointerEscape should be declared with the rest of the analysis result, including addressUsers and originalDestroys.
These are private to this small utility, initialized once by the analysis and subsequently immutable. That's nothing like a global variable!
That said, this utility performs two analysis steps, each with separate results. I usually separate each stage of results into separate classes. In fact, that was also my intention here. The first step, visitAddressUses, should be defined in MemAccessUtils. (This is the only code in this utility with substantial logic, so the utility truly becomes trivial at that point). I didn't do this yet because the existing AccessPath API is massive overkill for this case, and I didn't have time to refactor MemAccessUtils such that we can have a simpler API for discovering SSA address uses when we don't care about the access path.
There was a problem hiding this comment.
For sure, those member variables are like global variables within the algorithm. It's not clear what variables are set and used, e.g. by looking at HoistDestroyAddrs::perform().
Why not let the functions in HoistDestroyAddrs::perform() return the results and pass inputs as parameters? That would make the flow of information clear.
Until swiftlang#40392 lands, run destroy hoisting with lexical lifetimes.
atrick
force-pushed
the
destroy-hoist
branch
2 times, most recently
from
4d9b3e0 to
c5cdf14
Compare
Until swiftlang#40392 lands, run destroy hoisting with lexical lifetimes.
atrick
force-pushed
the
destroy-hoist
branch
2 times, most recently
from
7de9d1b to
ecf9d83
Compare
|
@swift-ci test |
atrick
changed the title
atrick
changed the title
|
@nate-chandler @eeckstein ShrinkBorrowScopes and SSADestroyHoisting should share as much logic and structure as we can. I extracted a few small shared SIL utilities from ShrinkBorrowScopes
I created a completely generic BackwardReachability utility as a more recognizable template for the kind of simple, efficient worklist data flow that we perform in many places. I could easily add an "optimistic" variation that iterates to a meet-over-paths fix point, but this "pessimistic" variation is more urgently needed for utilities that are called frequently to canonicalize the SIL. I extracted a UniqueStorageVisitor API that analyzes and classifies the leaf uses of unique storage (generally alloc_stack or indirect function arguments). Essentially just a switch over recognized opcode patterns. This is a pattern that we use in many analyses where we query the known uses of unique address storage. The sort of thing we do in CopyForwarding, TempRValue, and several more specific optimizations. This is urgently needed so we can add recognition of temporary copies. That will allow adding logic to SSADestroyHoisting to skip deinit barriers the same way that ShrinkBorrowScopes currently does. It could also be used to cleanup lots of other passes. The SSADestroyHoisting pass itself now only has logic for rewriting the destroys. It strings together two utilities to provide information to the rewrite step: UniqueStorageUseVisitor -> KnownUses -> BackwardReachability -> DeinitBarriers -> RewriteDestroys |
Needs to be common across ShrinkBorrowScopes and SSADestroyHoisting.
Analyze and classify the leaf uses of unique storage. Storage that has a unique set of roots within this function includes alloc_stack, alloc_box, exclusive argument, and global variables. All access to the storage within this function is derived from these roots. Gather the kinds of uses that are typically relevant to algorithms: - loads (including copies out of, not including inout args) - stores (including copies into and inout args) - destroys (of the entire aggregate) - debugUses (only populated when preserveDebugInfo == false)
Set the single-pass pipeline's isMandatory flag based on the opt-mode.
atrick
force-pushed
the
destroy-hoist
branch
from
ecf9d83 to
1db620e
Compare
|
@swift-ci smoke test |
Pessimistic, non-iterative data flow for analyzing backward reachability from a set of last uses to their dominating def or nearest barrier. Meet: ReachableEnd(predecessor) = intersection(ReachableBegin, successors) Intended for frequently called utilities where minimizing the cost of data flow is more important than analyzing reachability across loops. Expected to visit very few blocks because barriers often occur close to a last use. Note: this does not require initializing bitsets for all blocks in the function for each SSA value being analyzed.
Extract and rewrite the destroy hoisting algorithm originally from CopyForwarding (in 2014). This is now a light-weight utility for hoisting destroy_addr instructions. Shrinking an object's memory lifetime can allow removal of copy_addr and other optimization. This is extremely low-overhead and can run at any optimization level without dependency on any analysis. This algorithm is: - Incremental - SSA-based - Canonical - Free from alias analysis See file-level comments. The immediate purpose is to specify and test the constraints introduced by adding lexical variable lifetimes to SIL semantics. It can be used as a template for end_borrow hoisting. Ultimately, this utility can be invoked within any pass that needs to optimize a particular uniquely identified address. It will be used to remove much of the complexity from CopyForwarding.
atrick
force-pushed
the
destroy-hoist
branch
from
1db620e to
c8a2130
Compare
|
@swift-ci smoke test and merge |
Extract and rewrite the destroy hoisting algorithm originally from
CopyForwarding (in 2014).
This is now a light-weight utility for hoisting destroy_addr instructions. Shrinking an object's memory lifetime can allow removal of copy_addr and other optimization.
This algorithm is:
See file-level comments.
The immediate purpose is to specify and test the constraints introduced by adding lexical variable lifetimes to SIL semantics. It can be used as a template for end_borrow hoisting.
Ultimately, this utility can be invoked within any pass that needs to optimize a particular uniquely identified address. It will be used to remove much of the complexity from CopyForwarding.