PostAssemblyScript: Handle balanced retains involving allocations #2833
Conversation
|
While this should be good, I made this a draft for now just in case. Going to validate with the AS test suite and see if there's anything else to consider. |
| @@ -364,8 +364,16 @@ struct OptimizeARC : public WalkerPass<PostWalker<OptimizeARC>> { | |||
| return (*releaseLocation)->cast<Call>()->operands[0]->cast<LocalGet>(); | |||
| } | |||
|
|
|||
| // Tests if the given retain is generally eliminable (does not retain an | |||
| // allocation and does not reach an escape) | |||
| bool testRetainEliminable(LocalSet* retain, AliasGraph& graph) { | |||
There was a problem hiding this comment.
can the AliasGraph here be const AliasGraph&?
There was a problem hiding this comment.
Gave this a try, including updating the called functions herein, but the compiler then barks at me on [] accesses (no such operator) like graph.setInfluences[...] and graph.getSetses[...] in the called functions, as if std::map etc. can't be used this way. Might be missing something, as usual :)
There was a problem hiding this comment.
I think the issue is that map[key] can have side effects, it doesn't just do a read, but it also creates the entry (with a default value) if it isn't there.
You can do map.at(key) if you know the key must be there. that will not change the map, and it will also check it actually exists, so it's better (but again, only if you know the key must be there).
|
With the passes removed meanwhile, this PR is not necessary anymore. |
Adds handling of a new edge case to the PostAssemblyScript pass, with the pass no longer considering retains balanced if one of the retain patterns involved retains an allocation (allocations must never drop below RC=1 while in use). A new test case
keeps.balancedRetainsWithAllocationthat is otherwise identical toeliminates.balancedRetainsbut includes such an allocation verifies the expected behavior.