[PATCH] D140851: [Patch 3/4]: Add cases for assume (X & Y != {0|Y})

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked 2 inline comments as done.
goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:4832-4834
+  // Canonicalize:
+  // 1. A & B_Pow2 != B_Pow2 -> A & B_Pow2 == 0
+  // 1. A & B_Pow2 == B_Pow2 -> A & B_Pow2 != 0
----------------
spatel wrote:
> goldstein.w.n wrote:
> > spatel wrote:
> > > This should be split into an independent patch with several tests.
> > > 
> > > Double-check to make sure this is the same logic, but I think it'd be shorter and easier to read without the loop:
> > >   // If Op1 is a power-of-2 (has exactly one bit set):
> > >   // (Op1 & X) == Op1 --> (Op1 & X) != 0
> > >   // (Op1 & X) != Op1 --> (Op1 & X) == 0
> > >   if (match(Op0, m_c_And(m_Specific(Op1), m_Value())) &&
> > >       isKnownToBeAPowerOfTwo(Op1, false, 0, &I)) {
> > >     return new ICmpInst(CmpInst::getInversePredicate(Pred), Op0,
> > >                         ConstantInt::getNullValue(Op0->getType()));
> > >   }
> > >   // If Op0 is a power-of-2 (has exactly one bit set):
> > >   // Op0 == (Op0 & X) --> (Op0 & X) != 0
> > >   // Op0 != (Op0 & X) --> (Op0 & X) == 0
> > >   if (match(Op1, m_c_And(m_Specific(Op0), m_Value())) &&
> > >       isKnownToBeAPowerOfTwo(Op0, false, 0, &I)) {
> > >     return new ICmpInst(CmpInst::getInversePredicate(Pred), Op1,
> > >                         ConstantInt::getNullValue(Op0->getType()));
> > >   }
> > > 
> > > This should be split into an independent patch with several tests.
> > > 
> > Sure, if I split it off will I be able to just revise the parent/child status of the surrounding reviews to link it in or will I need to make 5-new ones?
> > 
> > > Double-check to make sure this is the same logic, but I think it'd be shorter and easier to read without the loop:
> > >   // If Op1 is a power-of-2 (has exactly one bit set):
> > >   // (Op1 & X) == Op1 --> (Op1 & X) != 0
> > >   // (Op1 & X) != Op1 --> (Op1 & X) == 0
> > >   if (match(Op0, m_c_And(m_Specific(Op1), m_Value())) &&
> > >       isKnownToBeAPowerOfTwo(Op1, false, 0, &I)) {
> > >     return new ICmpInst(CmpInst::getInversePredicate(Pred), Op0,
> > >                         ConstantInt::getNullValue(Op0->getType()));
> > >   }
> > >   // If Op0 is a power-of-2 (has exactly one bit set):
> > >   // Op0 == (Op0 & X) --> (Op0 & X) != 0
> > >   // Op0 != (Op0 & X) --> (Op0 & X) == 0
> > >   if (match(Op1, m_c_And(m_Specific(Op0), m_Value())) &&
> > >       isKnownToBeAPowerOfTwo(Op0, false, 0, &I)) {
> > >     return new ICmpInst(CmpInst::getInversePredicate(Pred), Op1,
> > >                         ConstantInt::getNullValue(Op0->getType()));
> > >   }
> > > 
> > 
> > Will take a look for next version.
> It's independent of anything else, so there's no need to update the other patches up for review? Just pull out this hunk and regenerate test diffs if needed. 
> 
> I just added some tests here:
> ad14cef1d530
> This should be split into an independent patch with several tests.
> 
> Double-check to make sure this is the same logic, but I think it'd be shorter and easier to read without the loop:
>   // If Op1 is a power-of-2 (has exactly one bit set):
>   // (Op1 & X) == Op1 --> (Op1 & X) != 0
>   // (Op1 & X) != Op1 --> (Op1 & X) == 0
>   if (match(Op0, m_c_And(m_Specific(Op1), m_Value())) &&
>       isKnownToBeAPowerOfTwo(Op1, false, 0, &I)) {
>     return new ICmpInst(CmpInst::getInversePredicate(Pred), Op0,
>                         ConstantInt::getNullValue(Op0->getType()));
>   }
>   // If Op0 is a power-of-2 (has exactly one bit set):
>   // Op0 == (Op0 & X) --> (Op0 & X) != 0
>   // Op0 != (Op0 & X) --> (Op0 & X) == 0
>   if (match(Op1, m_c_And(m_Specific(Op0), m_Value())) &&
>       isKnownToBeAPowerOfTwo(Op0, false, 0, &I)) {
>     return new ICmpInst(CmpInst::getInversePredicate(Pred), Op1,
>                         ConstantInt::getNullValue(Op0->getType()));
>   }
> 

Done (and used your code, its much less verbose), added you as reviewer but see: https://reviews.llvm.org/D141090


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140851