[Exclusivity] Remove dominated access checks with no nested conflict. #20053
Conversation
General case: — begin_access A (may or may not have no_nested_conflict) load/store end_access apply // may have a scoped access that conflicts with A begin_access A [no_nested_conflict] load/store end_access A — The second access scope does not need to be emitted. NOTE: KeyPath access must be identified at the top-level, non-inlinable stdlib entry point. As such, The sodlib entry pointed is annotated by a new @_semantics that is equivalent to inline(never)
|
@swift-ci Please test |
|
@atrick Can you please review? |
|
Build failed |
|
|
|
@swift-ci Please test OS X |
|
@swift-ci Please test |
|
@eeckstein I have added a new commit that makes the changes we discussed offline - mainly bailing out when encountering unpaired accesses and the new recursive walk that avoids explicitly checking for proper dominance: I went with a small vector of This vector keeps track of the first When encountering a When we are done with a sub-tree, we just have to remove the new elements in the small vector, which are the last entries we pushed back, this is efficiently implemented of course. |
|
Build failed |
|
@swift-ci Please test OS X |
|
Build failed |
|
@swift-ci Please smoke test OS X |
|
|
||
| // Finds domPairs for which we can change the dominated instruction to static | ||
| // NOTE: We might not be able to optimize some the pairs due to other | ||
| // restrictions Such as key-path or unpaired begin access We only traverse the |
There was a problem hiding this comment.
typo: missing "." after "access"
| // function once, if we find a pattern that *might* prevent optimization, we | ||
| // just add it to appropriate data structures which will be analyzed later. | ||
| // If we should bail on this function we return false - else true | ||
| // we bail to Simplify the code instead of handling unpaired accesses |
|
|
||
| // Restore the sets to their previous state as described above, | ||
| // removing all "new" elements | ||
| auto newNumOfElems = visitedDomAccessesToStorageInfo.size(); |
There was a problem hiding this comment.
please inline this expression into the assert. Otherwise you'll get a warning in the release build
| return false; | ||
| } | ||
|
|
||
| bool DominatedAccessRemoval::visitInstruction( |
There was a problem hiding this comment.
Please add a comment what the boolean return value means
| // we can turn the dominated to static during the optimize() phase | ||
| DomPairSet domPairs; | ||
| // All stdlib entry points that we've found in this function | ||
| KeyPathEntryPointsSet keypathEntries; |
There was a problem hiding this comment.
Any reason you are not handling the key path calls like unpaired accesses?
| SILFunction *callee = fullApply.getReferencedFunction(); | ||
| if (!callee) | ||
| return true; | ||
| if (!callee->hasSemanticsAttr("_keyPathEntryPoint")) |
There was a problem hiding this comment.
Is this the right check? Isn't it a decl attribute and not a semantic attribute?
| return true; | ||
| // we can't eliminate dominated checks even when we can prove that | ||
| // the dominated scope has no internal nested conflicts. | ||
| keypathEntries.insert(fullApply.getInstruction()); |
There was a problem hiding this comment.
Why not just return false here (and treat it like unpaired accesses)?
| // checks if the current storage has never been seen before | ||
| auto predNewStorageLoc = [&](AccessedStoragePair it) { | ||
| auto currStorage = it.second; | ||
| return !currStorage.isDistinctFrom(storage); |
There was a problem hiding this comment.
I think is is the wrong check. AFAIK, !isDistinct is not the inverse of locations being equal. And the equal-check is needed to be able to optimize the access.
| dealloc_stack %buffer : $*Builtin.UnsafeValueBuffer | ||
| %10 = tuple () | ||
| return %10 : $() | ||
| } |
There was a problem hiding this comment.
Please add a test with a key path
|
|
||
| bool DominatedAccessRemoval::analyzeDomSubTree( | ||
| SILBasicBlock *block, | ||
| AccessedStorageInfo &visitedDomAccessesToStorageInfo) { |
There was a problem hiding this comment.
It's okay that AccessedStorageInfo is a vector - assuming that it's small in the common case.
But you have to add a bail-out check if it gets larger than a certain limit, to avoid quadratic compile time in corner cases.
Also, you have to add a maximum-recursion-depth check because for very large functions, with lot of control flow, you can run into a stack overflow here.
|
@eeckstein I’ve done all the changes in the review, including adding recursion and quadratic compile time bail-outs, new key path test and bug-fix for the equal-check of accesses. I also changed the new attribute from a decl attribute to a semantic attribute |
Simplification of the code (bail on unpaired accesses) + change the implementation so we can avoid quadratic behavior
|
@swift-ci Please smoke test |
Radar rdar://problem/42789656
General case:
—
begin_access A (may or may not have no_nested_conflict)
load/store
end_access
apply // may have a scoped access that conflicts with A
begin_access A [no_nested_conflict]
load/store
end_access A
—
The second access scope does not need to be emitted.
NOTE: KeyPath access must be identified at the top-level, non-inlinable stdlib entry point.
As such, The sodlib entry pointed is annotated by a new @_semantics that is equivalent to inline(never)