[PATCH] D140798: [InstCombine] Fold zero check followed by decrement to usub.sat

[Nikita Popov via Phabricator via llvm-commits]

nikic added a reviewer: spatel.
nikic added a comment.

Proof: https://alive2.llvm.org/ce/z/a2jKgY

Is it possible/sensible to integrate this into canonicalizeSaturatedSubtract()? It seems like this is essentially the same, just for the `ult 1` -> `eq 0` special case. But maybe that is more awkward than a separate fold.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:869
+  if (match(A, m_Zero()))
+    std::swap(A, B);
+
----------------
Not necessary, icmp constants are canonicalized to the right.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:875
+  if (match(FalseVal, m_Zero()))
+    std::swap(TrueVal, FalseVal);
+
----------------
This is incorrect: If we swap select operands, we also need to invert the predicate. What you want to do is check whether the predicate is `ne` and then invert and swap. I believe this can only happen in multi-use scenarios, e.g.: https://llvm.godbolt.org/z/5Pxo3f8WE


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:881
+  Value *One = ConstantInt::get(A->getType(), 1);
+  Value *NegOne = Builder.CreateNeg(One);
+  if (match(FalseVal, m_Sub(m_Specific(A), m_Specific(One))) ||
----------------
It probably makes more sense to use m_SpecificInt here. While this CreateNeg call will not actually create an instruction, it looks like it could.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:882
+  Value *NegOne = Builder.CreateNeg(One);
+  if (match(FalseVal, m_Sub(m_Specific(A), m_Specific(One))) ||
+      match(FalseVal, m_Add(m_Specific(A), m_Specific(NegOne)))) {
----------------
Limited to just the constant case, I don't think we need to handle sub at all, it will always be canonicalized to add.


================
Comment at: llvm/test/Transforms/InstCombine/saturating-add-sub.ll:533
+  ret i8 %i2
+}
+
----------------
Especially if this is implemented as a separate fold, we'd want some negative test cases here, e.g. incorrect constants (vary zero/one), incorrect operands (not %a both time), incorrect icmp predicate.

As you are using `m_Zero()` matchers, which allow undef/poison in vectors, we'd want to test that as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140798