[PATCH] D154126: [InstCombine] Transform (A > 0) | (A < 0) -> zext (A != 0) fold

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

Other than a few nits, looks basically good.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1723
+
+  auto MatchOrZextIcmp = [&](Value *Op0, Value *Op1) -> bool {
+    return match(Op0, m_LShr(m_Value(A), m_APInt(B))) &&
----------------
nit: `ZextIcmp` -> `ZExtICmp`.


================
Comment at: llvm/test/Transforms/InstCombine/and-or-icmps.ll:2642
 
 define i64 @icmp_slt_0_or_icmp_sgt_0_i64_neg3(i64 %x) {
 ; CHECK-LABEL: @icmp_slt_0_or_icmp_sgt_0_i64_neg3(
----------------
nit, can you replace `<test>_neg<N>` -> `<test>_fail<N>` (neg can also be 0-X).


================
Comment at: llvm/test/Transforms/InstCombine/and-or-icmps.ll:2679
   %B = icmp sgt <2 x i64> %x, zeroinitializer
   %C = lshr <2 x i64> %x, <i64 62, i64 62>
   %D = zext <2 x i1> %B to <2 x i64>
----------------
you already test incorrect shift amount, maybe keep this one with correct shift amount but change the icmp to `sgt X, 1` (or any non-zero).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154126