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

[Renato Golin via llvm-commits]

rengolin added a subscriber: rengolin.
rengolin added a comment.

In http://reviews.llvm.org/D13259#260230, @anemet wrote:

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


Hi Adam,

I believe this is the case I'm looking at, from Livermore Loops, for the RAR case:

  a[k] = b[k] + b[k+1];

If the loop cannot be vectorized because of other problems, the LLE pass should be able to catch it, too, like the GVN does today, so best of both worlds.

Regarding this patch, I have some small comments, but overall, looks great!

I'll merge these changes locally and see if I can come up with a few RAR examples to transform.

cheers,
--renato


================
Comment at: lib/Transforms/Scalar/LoopLoadElimination.cpp:11
@@ +10,3 @@
+// This file implement a loop-aware load elimination pass.  It aims to find
+// store-to-load forwarding opportunies across the loop back-edge.
+//
----------------
Maybe not necessary to specify store-to-load, here, since this could also do load-to-load in the future?

================
Comment at: lib/Transforms/Scalar/LoopLoadElimination.cpp:153
@@ +152,3 @@
+      auto *Store = dyn_cast<StoreInst>(Source);
+      if (!Store)
+        continue;
----------------
I'm assuming to add LAL deps we'd need to add the case here, and map the type together, so that you can use the right struct (StoreToLoadForwardingCandidate or LoadToLoadForwardingCandidate). 

Or maybe just extend to ForwardingCandidate and add a type to it would be simpler.

================
Comment at: lib/Transforms/Scalar/LoopLoadElimination.cpp:194
@@ +193,3 @@
+  /// LAA will perform dependence analysis here because there are two
+  /// *different* pointers involved in the same alias set (&A[i] and &A[i+1]).
+  void removeDependencesFromMultipleStores(
----------------
Is this related to D13255?

================
Comment at: lib/Transforms/Scalar/LoopLoadElimination.cpp:319
@@ +318,3 @@
+    SmallSet<Value *, 4> PtrsWrittenOnFwdingPath =
+        computePointersWrittenOnForwardingPath(Candidates);
+
----------------
Silly nitpick: I originally thought this function would actually find the forward dependencies, not just find the candidates. Maybe changing the name to findPointers... would be more clear.

================
Comment at: lib/Transforms/Scalar/LoopLoadElimination.cpp:368
@@ +367,3 @@
+    Value *Ptr = Cand.Load->getPointerOperand();
+    auto *PtrSCEV = cast<SCEVAddRecExpr>(SE->getSCEV(Ptr));
+    auto *PH = L->getLoopPreheader();
----------------
would this be meaningful?

    assert(PtrSCEV)


http://reviews.llvm.org/D13259