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

[Noah Goldstein via Phabricator via Openmp-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:543
+    // Test scalar type arguments for conversion.
+    if (!isReducible(B, D, E))
+      return nullptr;
----------------
nit: maybe:
```
} 
// Test scalar type arguments for conversion.    
else if (!isReducible(B, D, E)) {
     return nullptr;
}
```

Although given the LLVM style of '}' and 'else' on the same line its a bit out of place. No strong feelings so update if you see fit.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:610
 /// Try to fold (icmp(A & B) ==/!= C) &/| (icmp(A & D) ==/!= E)
-/// into a single (icmp(A & X) ==/!= Y).
+/// into a single (icmp(A & X) ==/!= Y) or (icmp A u< Y).
 static Value *foldLogOpOfMaskedICmps(ICmpInst *LHS, ICmpInst *RHS, bool IsAnd,
----------------
When do we transform to `A & X eq/ne Y`? I don't see that code.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-logical.ll:2553
   ret <6 x i1> %t4
 }
----------------
One more thing. Can you put the "fail" keyword somewhere in the tests that you expect no change in. Think it helps make it crystal clear exact what the is meant to happen.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125717