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

[Aditya Kumar via llvm-commits]

hiraditya added inline comments.

================
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 :)
> 
It is more like B and C combined post dominating 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)
For each Instruction in the InstructionsToHoist, the nearest common dominator (w.r.t. HoistPt) could change.

e.g.,
A -> B -> C (has I1)
B-> D (has I2)
A -> E (has I3).

And if I1, I2 and I3 have the same GVN.
In this case nearestCommonDominator(C, D) = B, and, nearestCommonDominator(B, E) = A.



================
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.
> 
> 
> 
Will do that. Thanks.

================
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).
> 
It seems DFS number is not always available in the dominator tree.  DFS numbers are updated only if there are too many (> 32) slow queries in GenericDomTree.h:468


http://reviews.llvm.org/D19338