Minor cleanup in ARCSequenceOpts #33578
Conversation
|
@swift-ci test |
|
Build failed |
|
Build failed |
|
@swift-ci test OS X platform |
|
@swift-ci test Linux platform |
|
Build failed |
|
@swift-ci test OS X platform |
|
Build failed |
|
@swift-ci test OS X platform |
| mergeSuccessors(Subregion, SubregionData); | ||
|
|
||
| // Then perform the region optimization. | ||
| NestingDetected |= SubregionData.processBottomUp( |
There was a problem hiding this comment.
Why does the call to SubregionData.processBottomUp go away in this commit?
Nevermind, I see the other instance of processBottomUp now that I expanded the diff.
|
Build failed |
| @@ -88,6 +89,19 @@ void LoopRegion::print(llvm::raw_ostream &os, bool isShort) const { | |||
|
|
|||
| os << " ucfh:" << (IsUnknownControlFlowEdgeHead? "true " : "false") | |||
| << " ucft:" << (IsUnknownControlFlowEdgeTail? "true " : "false"); | |||
|
|
|||
| if (isVerbose) { | |||
There was a problem hiding this comment.
Can you use early returns rather than else if? That is:
if (!verbose)
return;
os << "\n";
if (isBlock())
return getBlock()->dump();
if (isLoop())
return getLoop()->dump();
if (isFunction())
return getFunction()->dump();
llvm_unreachable("Unknown region type");
In general, else if chains like this should be eliminated through the usage of early returns like that.
There was a problem hiding this comment.
Yes! Sorry I needed to be reminded :)
| @@ -285,9 +285,6 @@ handleRefCountInstMatch(SILInstruction *RefCountInst) { | |||
| return false; | |||
| case LatticeState::Decremented: | |||
| case LatticeState::MightBeUsed: | |||
| // Unset InsertPt so we remove retain release pairs instead of | |||
| // performing code motion. | |||
There was a problem hiding this comment.
I don't understand this change. Was there originally code here that was removed, but the comment wasn't removed?
There was a problem hiding this comment.
Yes, there were a lot of stale comments left out about 'InsertPt'. This code was refactored out and a lot of functionality moved to ARCCodeMotion.
| if (!isARCSignificantTerminator(&cast<TermInst>(*II))) { | ||
| II++; | ||
| } | ||
| if (!isARCSignificantTerminator(&cast<TermInst>(*II))) { |
There was a problem hiding this comment.
Maybe put a small comment here that we know that we have a TermInst since rbegin is the last instruction in the block and we are in well-formed canonical SIL?
| AA, RCFI, EAFI, LRFI, FreezeOwnedArgEpilogueReleases, IncToDecStateMap, | ||
| RegionStateInfo, SetFactory); | ||
| --End; | ||
| } |
There was a problem hiding this comment.
@meg-gupta do you know from the history why the last iteration was originally peeled?
There was a problem hiding this comment.
I can't tell much from history. Maybe just an oversight that it wasn't updated with getReverseSubregions.
There was a problem hiding this comment.
I remember why I think. I think there was a miscompile from clang with getReverseSubregions() when I wrote this. So I worked around it. Thanks for fixing!
| LLVM_DEBUG(llvm::dbgs() << "Adding new interesting insts!\n"); | ||
| do { | ||
| auto *I = NewInsts.pop_back_val(); | ||
| LLVM_DEBUG(llvm::dbgs() << " " << *I); |
There was a problem hiding this comment.
Just as a note to myself. The reason why this code was here was b/c we originally always performed code motion and were always rewriting all rr when making changes. The end result is that we needed to add the new instructions that we inserted to be adding to the interesting instructions inst. Since we are only removing RR now. This is not needed.
| // Returns true if any of the non-local successors of the region are leaking | ||
| // blocks. We currently do not handle early exits, but do handle trapping | ||
| // blocks. Returns false if otherwise | ||
| static bool hasEarlyExits( |
There was a problem hiding this comment.
Nice rename! Much better name! And thanks for the great comment improvement!
| @@ -48,7 +48,7 @@ llvm::cl::opt<bool> EnableLoopARC("enable-loop-arc", llvm::cl::init(false)); | |||
| // This routine takes in the ARCMatchingSet \p MatchSet and adds the increments | |||
| // and decrements to the delete list. | |||
| void ARCPairingContext::optimizeMatchingSet( | |||
There was a problem hiding this comment.
You may want to even rename this one to something more descriptive since we only delete instructions now. You could even make this a method on MatchSet. IDK, what are your thoughts.
meg-gupta
force-pushed
the
asominorchanges
branch
from
bae11c3 to
78dbefa
Compare
|
@swift-ci smoke test |
This change removes some more dead code in ARCSequenceOpts. And adds more info to the LoopRegion printer