[PATCH] D22778: Add Loop Sink pass to reverse the LICM based of basic block frequency.
Chandler Carruth via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 1 15:47:44 PDT 2016
chandlerc added a comment.
So, I don't think the code is quite ready to go into the tree yet. There are a bunch of minor issues that should be cleaned up. None of these are really big (it seems like you all have sorted out the algorithmic and high level design stuff), but I think they shuold be addressed before the code goes in. Especially the refactoring bit I suggest below.
================
Comment at: lib/Transforms/Scalar/LICM.cpp:438-447
@@ -441,4 +437,12 @@
///
-bool canSinkOrHoistInst(Instruction &I, AliasAnalysis *AA, DominatorTree *DT,
- TargetLibraryInfo *TLI, Loop *CurLoop,
- AliasSetTracker *CurAST, LoopSafetyInfo *SafetyInfo) {
+bool llvm::canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT,
+ Loop *CurLoop, AliasSetTracker *CurAST,
+ LoopSafetyInfo *SafetyInfo) {
+ if (!isa<LoadInst>(I) && !isa<CallInst>(I) &&
+ !isa<BinaryOperator>(I) && !isa<CastInst>(I) && !isa<SelectInst>(I) &&
+ !isa<GetElementPtrInst>(I) && !isa<CmpInst>(I) &&
+ !isa<InsertElementInst>(I) && !isa<ExtractElementInst>(I) &&
+ !isa<ShuffleVectorInst>(I) && !isa<ExtractValueInst>(I) &&
+ !isa<InsertValueInst>(I))
+ return false;
+
----------------
Can you split these refactorings into a separate patch please? They seem strictly beneficial and unrelated, and will make for example reverts messier if left in as part of this patch.
I have several minor and boring comments on the refactorings, but it seems better to make them on a dedicated patch than to clutter this thread with them.
(Just to be clear, I'd would leave it a static function, and just get the API you want and the implementation improvements. Then in this patch you can just make it an external function.)
================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:88-91
@@ +87,6 @@
+
+/// Return adjusted total frequency of BBs.
+/// If the size of BBs is > 1, sinking would lead to code size increase,
+/// which is tax by adding extra frequency to the total frequency.
+static BlockFrequency AdjustedSumFreq(DenseSet<BasicBlock *> &BBs,
+ BlockFrequencyInfo *BFI) {
----------------
I'm having a hard time understanding what this comment is trying to say. Can you try to re-word this to be more clear (and more clearly worded)?
================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:101
@@ +100,3 @@
+
+/// FindSinkBBs - Return a set of basic blocks that are:
+/// * Inside the loop
----------------
Since this is new code, please use the more modern form of doxygen throughout: http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
(I've also updated those to more accurately reflect that we use auto-brief new rather than explicit '\brief ...' sections.)
================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:101-105
@@ +100,7 @@
+
+/// FindSinkBBs - Return a set of basic blocks that are:
+/// * Inside the loop
+/// * Dominate BBs
+/// * Has minimum total frequency that is no greater than preheader frequency
+/// If no such set can be found, return an empty set.
+static DenseSet<BasicBlock *> FindSinkBBs(const Loop *L,
----------------
chandlerc wrote:
> Since this is new code, please use the more modern form of doxygen throughout: http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
>
> (I've also updated those to more accurately reflect that we use auto-brief new rather than explicit '\brief ...' sections.)
I feel like this comment too could be much more clear.
First, it isn't clear without a lot of context what the purpose of this would be. I'm guessing you mean something like find a candidate set of basic blocks to sink *into*?
"Dominate BBs" - this is ambiguous. Do *all* returned basic blocks need to dominate the set of blocks in BBs? Or is it more that for each block in BBs, at least one block in the returned set must dominate that block? A better name for the parameter than "BBs" would probably help here.
The frequency constraint isn't really explained. Why is that important? Give the reader more help to understand what the code will end up doing.
================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:106-107
@@ +105,4 @@
+/// If no such set can be found, return an empty set.
+static DenseSet<BasicBlock *> FindSinkBBs(const Loop *L,
+ const DenseSet<BasicBlock *> &BBs,
+ DominatorTree *DT,
----------------
Do you expect these sets to be large? Naively, I wouldn't...
If small is likely to be common, it would be better to use SmallPtrSetImpl as an input and SmallPtrSet as a result with a reasonable small size optimization.
================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:125-126
@@ +124,4 @@
+ SinkBBs.insert(BBs.begin(), BBs.end());
+ // Start from the coldest BB, if its frequency is no greater than all SinkBBs
+ // that are dominated it, replace them with the coldest BB.
+ for (BasicBlock *BB : SortedLoopBBs) {
----------------
This comment doesn't parse: "that are dominated it" seems to have a grammar error.
================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:127-137
@@ +126,13 @@
+ // that are dominated it, replace them with the coldest BB.
+ for (BasicBlock *BB : SortedLoopBBs) {
+ DenseSet<BasicBlock *> AddedBBs;
+ for (BasicBlock *B : SinkBBs)
+ if (DT->dominates(BB, B))
+ AddedBBs.insert(B);
+ if (AddedBBs.size() == 0)
+ continue;
+ if (AdjustedSumFreq(AddedBBs, BFI) > BFI->getBlockFreq(BB)) {
+ for (BasicBlock *B : AddedBBs) {
+ SinkBBs.erase(B);
+ }
+ SinkBBs.insert(BB);
----------------
I think this code needs significantly better variable names.
You have `BB`, `B`, and `B` again all for basic blocks in *different* containers.
And `AddedBBs` doesn't really tell the reader much about what the container is doing. Compare that to `SortedLoopBBs` which says exactly what it is. `SinkBBs` might also benefit from a slightly better name (and the function name might similarly benefit).
================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:128
@@ +127,3 @@
+ for (BasicBlock *BB : SortedLoopBBs) {
+ DenseSet<BasicBlock *> AddedBBs;
+ for (BasicBlock *B : SinkBBs)
----------------
Usually it is better to keep a set outside the loop and clear it on each iteration...
================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:158-160
@@ +157,5 @@
+ const BlockFrequency PreheaderFreq = BFI->getBlockFreq(Preheader);
+ bool HasColdBB = llvm::any_of(L->blocks(), [&](const BasicBlock *BB) {
+ return BFI->getBlockFreq(BB) <= PreheaderFreq;
+ });
+ if (!HasColdBB)
----------------
Why the variable rather than inlining this? Also, can you just call any_of directly since we have a using declaration and this is a range variant that doesn't exist in the standard?
================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:171-174
@@ +170,6 @@
+
+ // Putting all preheader instructions in a working list in reverse order.
+ // This maintains the integrety of the working set while some instructions
+ // sunk into loop body.
+ SmallVector<Instruction *, 10> INS;
+ for (auto II = Preheader->rbegin(), E = Preheader->rend(); II != E;) {
----------------
This comment again doesn't parse for me, but isn't this dead code now that you're just directly using the reverse iterators?
https://reviews.llvm.org/D22778
More information about the llvm-commits
mailing list