[PATCH] D68651: [InstCombine] Signed saturation patterns

[Dave Green via Phabricator via llvm-commits]

dmgreen marked 6 inline comments as done.
dmgreen added inline comments.


================
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:
> 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?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2043-2046
+  Type *NewTy = Type::getIntNTy(MinMax1.getContext(), NewBitWidth);
+  if (Ty->isVectorTy())
+    NewTy = VectorType::get(NewTy, Ty->getVectorNumElements(),
+                            Ty->getVectorIsScalable());
----------------
lebedev.ri wrote:
> This is ugly.
> There should be a way to do that by passing an old type and a new bitwidth.
Do you mean the whole "if (Ty->isVectorTy()).. "
or just the "VectorType::get(..)" part?

The second one looks simple, by using getElementCount. Let me know if you meant the first.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2058-2060
+  // And check the incoming are the correct type
+  if (A->getType()->getScalarType()->getIntegerBitWidth() > NewBitWidth ||
+      B->getType()->getScalarType()->getIntegerBitWidth() > NewBitWidth)
----------------
lebedev.ri wrote:
> I don't understand why this test is here.
> You know you matched `sext` on the way to `A` and `B`.
> 
I originally left this out of the new patch, but think it is still needed. We could be SExtending from something larger than the NewBitWidth, and if it does contain higher bits the results of the add might be different to what we expect.

Something like https://rise4fun.com/Alive/WZvW, if I have that correct (I don't know if there is a way to say "sadd_sat" exactly. Let me know if you know of a way, I thought I'd seen something like that before but can't find it now. The idea is to try and replicate what that does in the i9 part).


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

https://reviews.llvm.org/D68651