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

[Daniel Berlin via llvm-commits]

dberlin added a comment.

This looks much better to start. Thank you for working so hard on it.
More comments coming, but i made a first pass.


================
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.
----------------
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 :)


================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:425
@@ +424,3 @@
+      BasicBlock *BB = Insn->getParent();
+      BasicBlock *NewHoistPt = DT->findNearestCommonDominator(HoistPt, BB);
+      WL.insert(BB);
----------------
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)

================
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))
----------------
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.




================
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));
+
----------------
Note that you can get the DFS in/out numbers from the dominator tree if that is an acceptable ordering (IE DFS on DT).



http://reviews.llvm.org/D19338