[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;
----------------
Can you tell which test is not folded because of this check?



================
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());
----------------
This is ugly.
There should be a way to do that by passing an old type and a new bitwidth.


================
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,
----------------
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;
```



================
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)
----------------
I don't understand why this test is here.
You know you matched `sext` on the way to `A` and `B`.



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

https://reviews.llvm.org/D68651