[prev in list] [next in list] [prev in thread] [next in thread] 

List:       llvm-commits
Subject:    [PATCH] D140798: [InstCombine] Fold zero check followed by decrement to usub.sat
From:       Nikita Popov via Phabricator via llvm-commits <llvm-commits () lists ! llvm ! org>
Date:       2022-12-31 9:55:32
Message-ID: OeALpfKEQ8uu7q3qZyPDBw () geopod-ismtpd-4-0
[Download RAW message or body]

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

_______________________________________________
llvm-commits mailing list
llvm-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits


[prev in list] [next in list] [prev in thread] [next in thread]