[Exclusivity] Handle mayRelease instructions conservatively in AccessnforcementOpts and LICM #19119
Conversation
| @atrick Can you please review? |
| @swift-ci Please test |
There was a problem hiding this comment.
Is "scop" a typo or a term I don't know?
There was a problem hiding this comment.
Whoops! typo :) should be scope of course - thanks!
| @swift-ci Please test |
atrick left a comment
atrick left a comment There was a problem hiding this comment.
Your mayRelease changes look good.
Two somewhat separate areas for improvement in the existing code:
-
Use MapVector::remove_if instead of erase. Then remove all extra
while (changed)loops, which are quadratic and complicate the code. They are invisitSetForConflicts,detectApplyConflicts, andvisitBeginAccess. Also, remove the temporary map and extra loop inmergeAccessStruct. -
Remove the temporary map and extra loop in
recordConflict.
There was a problem hiding this comment.
It took me a while to convince myself this is correct. You've added an assumption at this point in the code that all possibly releasing instructions are included in MayWrites. We should add a corresponding comment where we actually compute MayWrites (in analyzeCurrentLoop). Also, note that even though we are only concerned with class or global access, we need to consider local deinitializers because they can refer to globals.
…EnforcementOpts and LICM
1) use vectors instead of MapVector for in/out of scope local maps 2) record nested conflict without creating a temporary copy
| @swift-ci Please test |
| I changed the code per our offline discussion - we no longer use |
| /// Reachability results are stored here because very few accesses are | ||
| /// typically in-progress at a particular program point, | ||
| /// particularly at block boundaries. | ||
| using DenseAccessMap = llvm::SmallMapVector<unsigned, BeginAccessInst *, 4>; |
There was a problem hiding this comment.
I think it would make more sense to call this DenseAccessSet (the vector part is an implementation detail of the Set).
| auto index = it->first; | ||
| info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.erase(index); | ||
| auto *otherBegin = *it; | ||
| auto rmIt = std::find( |
There was a problem hiding this comment.
Why is this now using find instead of conflictFreeAccesses.erase(otherBegin)?
There was a problem hiding this comment.
You can't use erase on a non-iterator type. it is a reverse iterator.
| accessStruct.conflictFreeAccesses.erase(index); | ||
| } | ||
| auto pred = [&](BeginAccessInst *it) { | ||
| auto rhsIt = std::find(RHSAccessStruct.conflictFreeAccesses.begin(), |
There was a problem hiding this comment.
Checking Set membership is done with count.
| info.inScopeConflictFreeAccesses.conflictFreeAccesses.insert( | ||
| std::make_pair(index, beginAccess)); | ||
| assert( | ||
| std::find(info.inScopeConflictFreeAccesses.conflictFreeAccesses.begin(), |
There was a problem hiding this comment.
Set membership is done with count.
| auto &ai = result.getAccessInfo(*it); | ||
| ai.setSeenNestedConflict(); | ||
| } | ||
| accessStruct.conflictFreeAccesses.erase(it); |
There was a problem hiding this comment.
This still invalidates the iterator.
There was a problem hiding this comment.
Hmm yes: it might invalidate the iterator in recordConflict's caller. not it in this function of course. I reverted to the old behavior in the caller
atrick left a comment
atrick left a comment There was a problem hiding this comment.
This doesn't look correct.
| Fixed #19140 |
3 participants
radar rdar://problem/42841839