[PATCH] D110817: [GVN] Simple GVN hoist - scalars

[Max Kazantsev via Phabricator via llvm-commits]

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