[PATCH] D68651: [InstCombine] Signed saturation patterns

[Sanjay Patel via Phabricator via llvm-commits]

spatel added subscribers: xbolva00, craig.topper.
spatel added a comment.

In D68651#1701189 <https://reviews.llvm.org/D68651#1701189>, @dmgreen wrote:

> Yes. I was going off the prior art for adding sadd_sat and ssub_sat to instcombine. And from the cases this patch is matching, where we are otherwise extending to a higher type, the intrinsic seems to produce equal or better code in most of the cases I've tried now. So this wasn't just for vectorisation, although it does make things a lot simpler there. (On an arm specific note, we have a scalar qadd instruction that can be used, if we can sort out the "q" flag otherwise being visible from C).
>
> If the canonical form for one of these signed saturating adds/subs wasn't an intrinsic, what would it be? Going into a higher type is awkward for us because the i64 add is not legal, and so doesn't look like the kind of instruction that should be vectorised, plus in ISel we'd have to catch the lowering fairly early and do something special.


I haven't looked at the patch in detail, but as author of at least part of the prior art cited here, I agree with the direction*. I also participated in some of the vector idioms discussions from a few years ago. There's overlap with the vector idiom problems, but as noted, these are generic (scalar too) math ops, so it's not exactly the same. We invested significantly in IR analysis and codegen for the math intrinsics, so that may have changed the thinking. I don't remember the sequence of events or if there was a dedicated llvm-dev thread for this, but the general idea is that if we have a generic intrinsic for the math and can easily invert the transform in the backend for targets/types that are not supported, try to canonicalize to the intrinsic.

https://bugs.llvm.org/show_bug.cgi?id=43580 may show another example where transforming to an intrinsic would help. cc @craig.topper @xbolva00 
*We should have codegen tests in place for multiple targets/types, and it appears that is added with D68643 <https://reviews.llvm.org/D68643>.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68651/new/

https://reviews.llvm.org/D68651