[PATCH] D110817: [GVN] Simple GVN hoist - scalars
Max Kazantsev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 5 23:06:11 PDT 2021
mkazantsev added inline comments.
================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:3074
+ if (!HasHoistBarrier || isSafeToSpeculativelyExecute(&I))
+ HoistPairs[VN.lookupOrAdd(&I)].first = &I;
+
----------------
chill wrote:
> mkazantsev wrote:
> > What worries me is that when we meet a non-guaranteed-to-transfer-execution instruction, we add it to HoistPairs before we actually raise the flag. Does it mean we can sometimes hoist such instructions?
> Yes, that's right, we can hoist such instructions. Should be fine, as we only hoist/merge *pairs* from the two sibling blocks.
>
> In principle, we can hoist (some) following instructions as well, if the two "barrier" instructions are themselves hoisted, but that's sort of expensive to check, so that's a limitation of the algorithm.
>
Then you have a problem with ICF (see comment below).
================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:3077
+ if ((HasHoistBarrier |= isHoistBarrier(I)))
+ LLVM_DEBUG(dbgs() << "Simple GVNHoist: reached hoist barrier" << I
+ << '\n';);
----------------
chill wrote:
> mkazantsev wrote:
> > nit: space after "barrier".
> Printing the instruction adds two spaces.
That's tricky :)
================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:3171
+ ThenI->moveBefore(DestBB->getTerminator());
+ replaceInstruction(ElseI, ThenI);
+
----------------
Since you move instructions with implicit control flow (i.e. non guaranteed to pass execution), this s incorrect treatment of ICF. You need to remove both EiseI and ThenI from ICF *before* you move it, and then add the moved instruction to ICF in the new block.
================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:3182
+ bool Change = false;
+ for (BasicBlock *BB : depth_first(&F.getEntryBlock())) {
+ // Check we have a block of the desired shape.
----------------
mkazantsev wrote:
> chill wrote:
> > mkazantsev wrote:
> > > Why not RPOT?
> > That is to prevent hoisting an instruction more than once. Alternatively, we could track how many blocks/instructions up an instruction moves, but that entails an auxiliary data structure.
> >
> But RPOT is that guarantees you that it doesn't hoist twice. Note that RPOT stands for *reverse* post order traversal, which is topological up-down traversal. It has a property (on DAG) that all block's predecessors are handled before the block.
>
> Here is example:
> ```
> A
> | \
> B C
> | / \
> D E
> / \
> F G
> ```
>
> If you chose DFS traversal, it may take A B D F G C E. You could hoist (F, G) -> D when processing D and then (D, E) -> C when processing C, which is hoisting same instruction twice.
>
> RPOT will protect you from this.
Maybe not the best example because current implementation doesn't hoist D->C, but still... RPOT looks safer.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110817/new/
https://reviews.llvm.org/D110817
More information about the llvm-commits
mailing list