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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:535
+
+    for (unsigned i = 0; i != NumElts; ++i) {
+      const auto *BElt = BConst->getAggregateElement(i);
----------------
nit(style): `i` -> `I`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:549
+    return Builder.CreateICmp(ICmpInst::ICMP_ULT, A, D);
+  }
+  return nullptr;
----------------
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);


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:494
+         "Expected equality predicates for masked type of icmps.");
+  if (!IsAnd || PredL != ICmpInst::ICMP_EQ || PredR != ICmpInst::ICMP_NE)
+    return nullptr;
----------------
jmciver wrote:
> goldstein.w.n wrote:
> > As a TODO (or here if you want to) you could also do:
> > ```
> > `(icmp (A & B) == B) & (icmp (A & D) == D)` -> `(icmp (A & (B | D) == (B | D)`
> > ```
> > https://alive2.llvm.org/ce/z/sFcvJP
> This maybe already handled, but let me know if I am missing something: 
> 
> https://godbolt.org/z/WqnveefxP
> 
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp#L577
> 
You're right, for some reason thought it wasn't being handled.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:543
+          match(EElt, m_APInt(ECst)) && *DCst == *ECst &&
+          (isa<UndefValue>(BElt) ||
+           (BCst->countLeadingOnes() == DCst->countLeadingZeros()))) {
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125717