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

[Adam Nemet via llvm-commits]

anemet added a comment.

Thanks very much, Daniel, for looking at the patch.

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


It catches the tests that check store-to-load forwarding.  These are 6, 7 and 9.

Test 10 has both store-to-load and load-to-load forwarding.   This pass catches st->ld but not ld->ld.

I guess in order to catch ld->ld as well we'd need LAA to also track RAR loop-carried dependences.  That does not sound difficult.


================
Comment at: lib/Transforms/Scalar/LoopLoadElimination.cpp:65
@@ +64,3 @@
+
+    if (LoadPtrType->getPointerAddressSpace() !=
+        StorePtrType->getPointerAddressSpace())
----------------
dberlin wrote:
> 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)
Actually after looking at this again, these checks are not necessary.

Since we only include store->load dependences of known types in the candidates, we can't have mismatching address space or type because that would yield an unknown dependence.

Sorry about the confusion, I'll change these into asserts.

Also as a follow-on, I am planning to make dependence distance an attribute of the dependence class and then this function would go away.

================
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,
----------------
dberlin wrote:
> 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.
> 
I think you're right.  We should be able to enhance it further to also handle cases where to load is only partially redundant.

Needless to say, I am going for fully-redundant loads in this initial version.

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


http://reviews.llvm.org/D13259