[PATCH] D22777: GVN-hoist: use a DFS numbering of instructions (PR28670)

[Sebastian Pop via llvm-commits]

sebpop added inline comments.

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:787
@@ -793,1 +786,3 @@
+      // DFS numbers change when instructions are hoisted: clear and recompute.
+      DFSNumber.clear();
     }
----------------
dberlin wrote:
> Oh, this is the part i forgot :)
> Oh well.
> 
> One thought:
> 
> If we are always inserting at the end of a block (i didn't look), btw, we'd be better off having a per-block numbering.
> 
> If we make "I" (here) a member variable called LastDFSNumber, we can then just update the per-block numberings by using ++LastDFSNumber as the number for the hoisted instruction. This avoids walking the instruction stream again on every iteration.
> 
> 
> 
Yes, that would work.
I was also thinking along the lines of how we locally update the MSSA, we could also locally update the DFS numbers.
In that case we should compute both a DFS for BBs that will not change with hoisting, and then a DFS for instructions local to a BB and we could locally patch when hoisting instructions.

================
Comment at: llvm/test/Transforms/GVN/hoist.ll:638
@@ +637,3 @@
+; CHECK-NEXT: getelementptr
+; CHECK-NEXT: getelementptr
+; CHECK-NEXT: store
----------------
dberlin wrote:
> So wait, this hoists more than we did before?
No.  I added this missing gep because I changed the CHECK into a CHECK-NEXT, and we now pattern match all the basic block where we hoist.


https://reviews.llvm.org/D22777