[PATCH] D68651: [InstCombine] Signed saturation patterns

[Dave Green via Phabricator via llvm-commits]

dmgreen marked 3 inline comments as done.
dmgreen added a comment.

Thanks for the alive proofs. I had written a patch to do the same thing (for sat's at least), but it looks like you already have something similar that handles overflows too.

In D68651#1711777 <https://reviews.llvm.org/D68651#1711777>, @nikic wrote:

> @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.


I will look into this. Thanks for the pointers!



================
Comment at: llvm/include/llvm/IR/Type.h:388
+  /// whilst keeping the old number of lanes.
+  inline Type *changeBitWidth(unsigned NewBitwidth) const;
+
----------------
lebedev.ri wrote:
> Can you use it somewhere in existing code and land this change separately?
D69139, as you already saw.


================
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;
----------------
nikic wrote:
> 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.
InstCombine used to be more strict about changing to illegal types before shouldChangeType was relaxed to at least include common types. I've not ran performance numbers either way over whether it's better to be less strict about this. It likely doesn't come up a lot in this case.

The checks we currently have here will only allow generate sats if there was an existing value of that type, from the sext (I think). sext from a larger type will not be valid to transform, and if both inputs are smaller the min/max will be superfluous (so we just use a standard add). So at least one of the inputs needs to be the same size as the final sat.

Whether we should turn a sext from an illegal type into a sadd_sat with an illegal type is the question then. I'd still go with no, but that's mostly because it feel's simpler (and it's likely does not come up very often).


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

https://reviews.llvm.org/D68651