[PATCH] D68651: [InstCombine] Signed saturation patterns

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

@lebedev.ri I believe that pattern is principally handled in `canonicalizeSaturatedAdd()`, it just depends on the way in which the overflow check is expressed. Apparently it checks for the `~X u< Y ? -1 : X + Y` pattern, but not for `(X + Y) < X ? -1 : X + Y`, which is our canonical non-constant add overflow check form, I think.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2032-2035
+  // FIXME: This isn't quite right for vectors, but using the scalar type is a
+  // good first approximation for what should be done there.
+  if (!shouldChangeType(Ty->getScalarType()->getIntegerBitWidth(), NewBitWidth))
+    return nullptr;
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > dmgreen wrote:
> > > lebedev.ri wrote:
> > > > Can you tell which test is not folded because of this check?
> > > > 
> > > Ah. That was meant to be sadd_sat31, but then I re-added back in some extra checks. I'll change it to a sadd_sat4 which should test this.
> > > 
> > > I presume it's a good idea to prevent the creation i4's?
> > Eh, good point.
> > Not sure honestly, i don't remember ever seeing that check in any of the transforms recently-ish added.
> @spatel @nikic thoughts? I'm almost thinking this should not be here.
I think this check makes sense in this case, because we are creating a type from integer constants. E.g. if those happen to be `-256` and `255`, we'd create a `sadd.sat.i9` call, which will introduce the use of an "unusual" type that was not present in the source program. I think that's something we should generally avoid, as it will likely result in an unfavorable lowering in the backend.


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

https://reviews.llvm.org/D68651