[PATCH] D13259: LLE 6/6: Add LoopLoadElimination pass

[Daniel Berlin via llvm-commits]

dberlin added a comment.

Thanks so much for taking this on.

Out of curiosity, does this actually catch the PRE load-forwarding cases that currently are tested?

That is, tests/Transform/GVN/pre-load.ll tests 9 and 10 i believe.


================
Comment at: lib/Transforms/Scalar/LoopLoadElimination.cpp:65
@@ +64,3 @@
+
+    if (LoadPtrType->getPointerAddressSpace() !=
+        StorePtrType->getPointerAddressSpace())
----------------
This and the checks below it really look like checks to see if the load and store are really the same type, and not related to the dependence distance.
Can this be abstracted out somewhere?

IE it makes more sense to be calling Cand.isSameTypeAs(SE)  && Cand.iisDependenceDistanceOfOne(SE)

================
Comment at: lib/Transforms/Scalar/LoopLoadElimination.cpp:100
@@ +99,3 @@
+/// \brief Check if the store dominates all latches, so as long as there is no
+/// intervening store this value will be loaded in the next iteration.
+bool doesStoreDominatesAllLatches(BasicBlock *StoreBlock, Loop *L,
----------------
This is an interesting restriction, and i understand why it is here.
I haven't thought too hard about it, but  wonder if it could not be gotten rid of in a lot of cases with code or guard insertion, PRE style.


================
Comment at: lib/Transforms/Scalar/LoopLoadElimination.cpp:137
@@ +136,3 @@
+      Instruction *Destination = Dep.getDestination(LAI);
+
+      if (Dep.Type == MemoryDepChecker::Dependence::Unknown) {
----------------
You also could do the type filtering you are doing in isDependenceDistanceOfOne here, no?



http://reviews.llvm.org/D13259