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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n 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);
----------------
goldstein.w.n wrote:
> nikic wrote:
> > 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).
> > 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.
> > 
> Agreed that is a limitation, but seems to me be better than nothing no?
> 
> > 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).
> 
> Although if Y is constant then it will be no undef so it will reach `X & Y == Y` -> `X & ~Y == 0` either way.
> > 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.
> > 
> Agreed that is a limitation, but seems to me be better than nothing no?
> 
> > 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).
> 
> Although if Y is constant then it will be no undef so it will reach `X & Y == Y` -> `X & ~Y == 0` either way.




================
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);
----------------
goldstein.w.n wrote:
> goldstein.w.n wrote:
> > nikic wrote:
> > > 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).
> > > 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.
> > > 
> > Agreed that is a limitation, but seems to me be better than nothing no?
> > 
> > > 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).
> > 
> > Although if Y is constant then it will be no undef so it will reach `X & Y == Y` -> `X & ~Y == 0` either way.
> > > 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.
> > > 
> > Agreed that is a limitation, but seems to me be better than nothing no?
> > 
> > > 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).
> > 
> > Although if Y is constant then it will be no undef so it will reach `X & Y == Y` -> `X & ~Y == 0` either way.
> 
> 
> > 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.
> > 
> Agreed that is a limitation, but seems to me be better than nothing no?
> 
> > 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).
> 
> Although if Y is constant then it will be no undef so it will reach `X & Y == Y` -> `X & ~Y == 0` either way.

I'm wrong about this. Okay ill update to make only for constants and do the ~Y version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144610