[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:3182
+  bool Change = false;
+  for (BasicBlock *BB : depth_first(&F.getEntryBlock())) {
+    // Check we have a block of the desired shape.
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110817/new/

https://reviews.llvm.org/D110817