[Openmp-commits] [PATCH] D125717: [InstCombine] Optimize and of icmps with power-of-2 and contiguous masks

[John McIver via Phabricator via Openmp-commits]

jmciver marked 2 inline comments as done.
jmciver added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:549
+    return Builder.CreateICmp(ICmpInst::ICMP_ULT, A, D);
+  }
+  return nullptr;
----------------
goldstein.w.n wrote:
> I think it would be clearer to organize this as follows:
> 
> ```
> if(IsVec) {
> foreach(B, D, E) {
> if(!isReducible) { return nullptr; }
> }
> }
> else {
> if(!isReducible) { return nullptr; }
> }
> return ICmpInst(ULT, A, D);
I agree that is much cleaner. I have refactored accordingly. I added a comment to the function header stating that parameter B supports masking.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:543
+          match(EElt, m_APInt(ECst)) && *DCst == *ECst &&
+          (isa<UndefValue>(BElt) ||
+           (BCst->countLeadingOnes() == DCst->countLeadingZeros()))) {
----------------
goldstein.w.n wrote:
> jmciver wrote:
> > goldstein.w.n wrote:
> > > Can you add some tests with `undef` elements.
> > > 
> > > Also if `undef` is allowed in the non-vec case as well, this check and the scalar one could be done with a lambda. 
> > I added tests using undef splat vectors.
> > 
> > The conversion to a lambda is much cleaner. I have enabled support for undef/poison support in the scalar case. However, testing of this transformation using scalar undef or poison enables an earlier optimization path: 
> > 
> > https://godbolt.org/z/7c87ba45v
> > 
> > The following Alive2 output show the scalar use of undef and poison with this transformation to be correct:
> > 
> > https://alive2.llvm.org/ce/z/9_eQny
> Thanks. Just FYI, you can do multiple proofs in the same link by postfixing 'src' with either '\d+' or '_<desc>' i.e:
> https://godbolt.org/z/M5eK16Ess
Thanks for the Godbolt pointer!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125717