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

[Adam Nemet via llvm-commits]

anemet marked 6 inline comments as done.
anemet added a comment.

In http://reviews.llvm.org/D13259#275336, @rengolin 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.


Yes, you're right.  It looks like we will need RAR for your case as well.

Adam


================
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.
+//
----------------
rengolin wrote:
> Maybe not necessary to specify store-to-load, here, since this could also do load-to-load in the future?
Makes sense.

================
Comment at: lib/Transforms/Scalar/LoopLoadElimination.cpp:153
@@ +152,3 @@
+      auto *Store = dyn_cast<StoreInst>(Source);
+      if (!Store)
+        continue;
----------------
rengolin wrote:
> 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.
Yeah probably the latter since they should be pretty similar.  But I could be wrong.

================
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(
----------------
rengolin wrote:
> Is this related to D13255?
Yes, the comment is meant to explain why the issue I discovered in D13255 does not affect LLE.

================
Comment at: lib/Transforms/Scalar/LoopLoadElimination.cpp:319
@@ +318,3 @@
+    SmallSet<Value *, 4> PtrsWrittenOnFwdingPath =
+        computePointersWrittenOnForwardingPath(Candidates);
+
----------------
rengolin wrote:
> 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.
This comment may have shifted or I am just confused.  Can you please elaborate what you mean?

================
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();
----------------
rengolin wrote:
> would this be meaningful?
> 
>     assert(PtrSCEV)
cast has an implicit assert, no?


http://reviews.llvm.org/D13259