[PATCH] D117252: [InstCombine] Fold ashr-exact into a icmp-ugt.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

Logic looks good. 
Please pre-commit the new tests with baseline CHECK lines.
It would be nice to have at least one test with a vector type and splat constants, so we have coverage for that possibility. I'd also add at least one test with an extra use of the ashr, so we know that doesn't impede the transform.



================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2243-2244
+    if (Pred == CmpInst::ICMP_SLT || Pred == CmpInst::ICMP_ULT || IsExact) {
       // icmp slt (ashr X, ShAmtC), C --> icmp slt X, (C << ShAmtC)
       // icmp sgt (ashr exact X, ShAmtC), C --> icmp sgt X, (C << ShAmtC)
       APInt ShiftedC = C.shl(ShAmtVal);
----------------
This comment should be updated to be more general. Author's pref for notation/formatting, but it's something like this:
     // When ShAmtC can be shifted losslessly:
     // (X s>> ShAmtC) < C --> X < (C << ShAmtC)  (either signed/unsigned)
     // (X s_exact>> ShAmtC) [AnyPred] C --> X [AnyPred] (C << ShAmtC)



================
Comment at: test/Transforms/InstCombine/icmp-shr.ll:716
 
 ; negative test
 
----------------
Here and below: remove stale test comments to avoid confusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117252