[PATCH] D38948: [LV] Support efficient vectorization of an induction with redundant casts

[Dorit Nuzman via Phabricator via llvm-commits]

dorit added a comment.

> Yes, IndVarSimpify wouldn't fix this issue, but I was thinking more of using the techniques there that use the SCEV expressions to find these cases instead of doing the pattern matching (see the inline comment).

Oh, so your comment was referring to how we retrieve the IR sequence, not to how we vectorize the phi...  Ok, I think I understand your point now.

>> Do you mean this will not hold once additional forms of "casted-inductions" are supported by createAddRecFromPhiWithCasts()?
> 
> Yes, but even if it's not a correctness issue I think we should avoid pattern matching here if possible to avoid doing more work in the future.

Yes, I agree it's preferable.
The current pattern-matching is actually an overkill, just being overly cautious checking again things we don't need to re-check.  It's enough to walk the def-use chain starting from the value that defines the phi via the back-edge, and, as you suggest, once we encounter a Value on the way whose SCEV is the same as the PHI SCEV to mark instructions as part of a casting sequence.

I agree that it's nicer. I would hesitate about implementing here something much more generic than what our SCEV-rewriter is currently able to support. But it can certainly be made more generic than it currently is, so it will be easier to extend it even further, in the future, if/when the need arises. I will give it  try.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:4482
+///   x1 = phi (x0, x_next)
+///   x2 = ExtTrunc (x1)
+///   x_next = add (x2, Step)
----------------
sbaranga wrote:
> If I understand correctly the SCEV expression returned by PSE for x2 should be the same as for x1?
> 
> In that case, wouldn't it be possible to search for values in the loop with the same SCEV as an induction PHI, go through the def-use chain until you've reached the PHI and mark all instructions in between as a (part of) a cast of that PHI? I think this wouldn't need any pattern matching and should be straight-forward to do.
> 
> Also I think ideally this should all be in an utility function (LoopUtils?). It seems a strange to have this in Scalar Evolution. It's very specific to the vectorizer.
> If I understand correctly the SCEV expression returned by PSE for x2 should be the same as for x1?

Yes, that should be the case

> In that case, wouldn't it be possible to search for values in the loop with the same SCEV as an induction PHI

Is there a ScalarEvolution map that holds this information (all the Values that have the same SCEV)? There's the ExprValueMap which looks close, but I'm not sure... or do you suggest to really obtain "from scratch" all the values defined in the loop and call getSCEV for each? Would it make more sense to start from the Value that defines the phi from the backedge (the way we start now), walk the def-use chain from there? (calling getSCEV on each value we find on the way, and from the point where we encounter the same SCEV as the induction phi continue the def-use walk while marking instructions on the way as part of a cast)?

> Also I think ideally this should all be in an utility function (LoopUtils?). It seems a strange to have this in Scalar Evolution. It's very specific to the vectorizer.
Ok, sure, will move it.


 


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4491
+///   ExtTrunc2: 
+///     Cast:  %t = shl %x1, m
+///     Cast:  %x2 = ashr %t, m
----------------
sbaranga wrote:
> Technically this would be part of a cast operation, and not a cast itself? (shl changes the value).
Yes, sure. It's a "Cast-Sequence Member". I'll drop the "Cast:" from the comment. 


https://reviews.llvm.org/D38948