[PATCH] D68651: [InstCombine] Signed saturation patterns

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri 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;
----------------
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.


================
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());
----------------
dmgreen wrote:
> 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.
The entirety of the block i highlighted.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2048-2057
+  Value *A, *B;
+  Function *F;
+  if (match(AddSub, m_Add(m_SExt(m_Value(A)), m_SExt(m_Value(B)))))
+    F = Intrinsic::getDeclaration(MinMax1.getModule(), Intrinsic::sadd_sat,
+                                  NewTy);
+  else if (match(AddSub, m_Sub(m_SExt(m_Value(A)), m_SExt(m_Value(B)))))
+    F = Intrinsic::getDeclaration(MinMax1.getModule(), Intrinsic::ssub_sat,
----------------
lebedev.ri wrote:
> This should be closer to something like
> ```
>   Value *A, *B;
>   if(!match(AddSub, m_BinOp(m_SExt(m_Value(A)), m_SExt(m_Value(B)))))
>     return nullptr'
> 
>   Function *F;
>   if (AddSub.getOpcode() == Instruction::Add)
>     F = Intrinsic::getDeclaration(MinMax1.getModule(), Intrinsic::sadd_sat,
>                                   NewTy);
>   else if (AddSub.getOpcode() == Instruction::Sub)
>     F = Intrinsic::getDeclaration(MinMax1.getModule(), Intrinsic::ssub_sat,
>                                   NewTy);
>   else
>     return nullptr;
> ```
> 
Now that i think about it, the only really differing part is the new opcode, so
```
  ZZZ *NewOpcode;
  if (AddSub->getOpcode() == Instruction::Add)
    NewOpcode = Intrinsic::sadd_sat;
  else if (AddSub->getOpcode() == Instruction::Sub)
    NewOpcode = Intrinsic::ssub_sat;
  else
    return nullptr;

  auto* F = Intrinsic::getDeclaration(MinMax1.getModule(), Intrinsic::ssub_sat, NewTy);

```
even.


================
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)
----------------
dmgreen wrote:
> 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).
Please move to right after `if(!match(AddSub, m_BinOp(m_SExt(`.
I guess i see what you mean, please ensure there is a test for that.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2031
+    return nullptr;
+  unsigned NewBitWidth = (*MaxValue + 1).logBase2() + 1;
+  // FIXME: This isn't quite right for vectors, but using the scalar type is a
----------------
```
/// In what bitwidth can this be treated as saturating arithmetics?
```


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2061
+
+  // And check the incoming are the correct type
+  if (A->getType()->getScalarType()->getIntegerBitWidth() > NewBitWidth ||
----------------
This needs more meaningful comment.


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

https://reviews.llvm.org/D68651