[PATCH] D33058: [LV] Sink casts to unravel first order recurrence

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

In https://reviews.llvm.org/D33058#751569, @mssimpso wrote:

> Hi Ayal,
>
> Thanks for extending this. The current dominance requirement can be fairly restrictive. Is there anything special about casts? I'm wondering if it's worth extending this to handle other kinds of instructions/expressions.


The case of casts arises naturally when GVN::PerformLoadPRE() optimizes an a[i]+a[i+1] where "a" is an array e.g. of type short and the addition is of type int, as in the attached test-case. It is also possible though to apply this optimization manually to the source code, by doing:

  short ai = a[0];
  for (int i = 0; i < n; ++i) {
    short aiPlusOne = a[i+1];
    b[i] = ai * aiPlusOne;
    ai = aiPlusOne;
  }

or manually add **any** instruction to use Phi before Previous, such as the following "+= 3":

  int ai = a[0];
  for (int i = 0; i < n; ++i) {
    ai += 3;
    int aiPlusOne = a[i+1];
    b[i] = ai * aiPlusOne;
    ai = aiPlusOne;
  }

(Doing "b[i]=(a[i]+3)+a[i+1]" btw produces a cast followed by an add, both above Previous; a test-case @aemerson asked for.)

In https://reviews.llvm.org/D33058#752015, @aemerson wrote:

> In general I wonder if this is really the best place to do this. It would be nice if the loop was canonicalised to be in this form given how cheap it is to do. Perhaps LoopSimplify? Not blocking this change, but something to think about.


The specific a[i]+a[i+1] case could indeed be handled before vectorization, namely by (LoopSimplify?) hoisting the cast along with the load. This applies in general to other instructions that operate symmetrically on both a[i] and a[i+1]. Instructions that only apply to a[i] need to be carefully placed inside the loop in order for it to be vectorized. It's probably better to have the vectorizer handle such motion; nothing keeps these instructions from returning back to their original positions.

Sounds reasonable?



================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:556
+  if (!Previous || !TheLoop->contains(Previous) || isa<PHINode>(Previous) ||
+      SinkAfter.count(Previous)) // Cannot rely on dominance due to motion.
     return false;
----------------
aemerson wrote:
> In what situation can this occur? Isn't this an output vector?
In general, once instructions are allowed to move, their updated location and dominance info should be considered. Furthermore, if transitive motion is allowed, e.g., sink a after b and sink b after c, they need to execute in the right order.

In the scenarios encountered Previous is a load, so this does not occur, as sinking is restricted here to casts.

Note that Interleave Groups also move instructions, but w/o affecting such sinking of casts: loads move upwards (w/o breaking their original dominance info) and stores move downwards (having no users).


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:564
+    auto *I = Phi->user_back();
+    if (I->isCast() && (I->getParent() == Phi->getParent()) && I->hasOneUse() &
+        DT->dominates(Previous, I->user_back())) {
----------------
aemerson wrote:
> Bug here, bitwise `&` used. Which is also telling me that there aren't sufficient tests for this, especially the case where Previous doesn't dominate the cast's user and so the cast can't be legally sunk past Previous.
Right, thanks!


================
Comment at: test/Transforms/LoopVectorize/first-order-recurrence.ll:401
+; SINK-AFTER-LABEL: sink_after
+; SINK-AFTER: vector.body
+; void sink_after(short *a, int n, int *b) {
----------------
aemerson wrote:
> Can we add a few more checks to verify the result of the vectorization is sane?
Sure.


https://reviews.llvm.org/D33058