[PATCH] D19338: New code hoisting pass based on GVN (optimistic approach)

[Sebastian Pop via llvm-commits]

sebpop added inline comments.

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:137-152
@@ +136,18 @@
+public:
+  void insert(CallInst *Call, GVN::ValueTable &VN) {
+    if (Call->doesNotReturn() || !Call->doesNotThrow())
+      return;
+
+    // A call that doesNotAccessMemory is handled as a Scalar,
+    // onlyReadsMemory will be handled as a Load instruction,
+    // all other calls will be handled as stores.
+    unsigned V = VN.lookupOrAdd(Call);
+
+    if (Call->doesNotAccessMemory())
+      VNtoCallsScalars.insert(std::make_pair(V, Call));
+    else if (Call->onlyReadsMemory())
+      VNtoCallsLoads.insert(std::make_pair(V, Call));
+    else
+      VNtoCallsStores.insert(std::make_pair(V, Call));
+  }
+
----------------
majnemer wrote:
> What if the call is to a function which divides by zero?  What would stop you from hoisting the call to it?
We do check for no EH and no BB address taken in between the original place of the expressions and the place we hoist it to.
After hoisting, if the call throws an exception it would happen a bit earlier than it used to,
although there shouldn't be any other exceptions or side effects happening in between the new and old place of the call.

We also check that all paths from the new location to the end of the function do have the exact call that we hoist.
So the call with the exception has to happen on all paths.

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:213
@@ +212,3 @@
+
+  // Return true when all paths from A to the end of the function pass through
+  // either B or C.
----------------
dberlin wrote:
> Errr, isn't this the definition of A post-dominating B or C?
> 
> 
> A post-dominates B if all paths from A to end of function pass through B.
> Same with (A, C).
> 
> If that's right, i would just use post-dominance here :)
> 
Post-Dominance is computed on the reversed-CFG: (turning each edge in the opposite direction)
A post-dominates B if all paths from the end of the function to B on the reversed-CFG pass through A.


================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:425
@@ +424,3 @@
+      BasicBlock *BB = Insn->getParent();
+      BasicBlock *NewHoistPt = DT->findNearestCommonDominator(HoistPt, BB);
+      WL.insert(BB);
----------------
dberlin wrote:
> I wonder how expensive this computation ends up being. It never changes per-iteration unless something is messing with the CFG out from under you.
> 
> (This is why GCC uses et-splay trees)
The good thing is that the hoist pass does not change the CFG.
Overall compilation time is also 0.35% increase on x86_64 compiling all the test-suite through valgrind (see some of the previous experiments.)

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:635-637
@@ +634,5 @@
+            continue;
+          if (!OptForMinSize && !Call->onlyReadsMemory())
+            CallWithSideEffect = true;
+          CI.insert(Call, VN);
+        } else if (!CallWithSideEffect && !isa<GetElementPtrInst>(&I1))
----------------
dberlin wrote:
> majnemer wrote:
> > Likewise, you need to make sure that the call has no side effects which is different from it not mutating memory.
> +1 
> Pure and const (in gcc parlance) are pretty much the only thing you can safely move.
> 
> 
> 
I will add a check for that.  Thanks for catching this.

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:665
@@ +664,3 @@
+    for (const BasicBlock *BB : depth_first(&F.getEntryBlock()))
+      DFSNumber.insert(std::make_pair(BB, ++I));
+
----------------
dberlin wrote:
> Note that you can get the DFS in/out numbers from the dominator tree if that is an acceptable ordering (IE DFS on DT).
> 
I will look at that.  I remember Aditya had a patch doing that: he needed to expose that numbering from the DT construction through a new function. From what I remember we reverted back to this numbering after some problem with that numbering.  I will try to see if I can remove this loop from the patch.


http://reviews.llvm.org/D19338