[PATCH] D144610: [InstCombine] Add transforms for `(icmp upred (or X, Y), X)`

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:4127-4133
+  // icmp (X | noundef Y) eq/ne X
+  //    if (X | noundef Y).hasOneUse()
+  //        --> (X & noundef Y) eq/ne noundef Y
+  if (ICmpInst::isEquality(Pred) && Op0->hasOneUse() &&
+      isGuaranteedNotToBeUndefOrPoison(A, Q.AC, Q.CxtI, Q.DT,
+                                       /*Depth*/ 0))
+    return new ICmpInst(Pred, IC.Builder.CreateAnd(Op1, A), A);
----------------
I'm somewhat skeptical about this fold. I can see where you're coming from here, in terms of picking a canonical form between and/or for this pattern, but the fact that we need the undef limitation here (so the canonicalization will be fairly unreliable) and that and/or are generally always handled in conjugate patterns (i.e. we shouldn't have cases where one optimizes better than the other) makes this fold not very compelling to me.

The more obvious canonicalization choice would be `X & ~Y == 0`, but that adds an extra instruction. We do canonicalize this for the case where Y is a constant, but don't do it even if Y is freely invertable (https://llvm.godbolt.org/z/7MWefMbP9).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144610