[PATCH] D144248: [InstCombine] canonicalize urem as cmp+select (part 2)

[Allen zhong via Phabricator via llvm-commits]

Allen added a comment.

In D144248#4139622 <https://reviews.llvm.org/D144248#4139622>, @nikic wrote:

> Compile-time: https://llvm-compile-time-tracker.com/compare.php?from=45a0b812fa13ec255cae91f974540a4d805a8d79&to=787c3e2333e8d655a61ef013f7006d7675c03574&stat=instructions%3Au Some impact, but less than I expected, so I think we can do this.
>
> This patch should also remove the isImpliedByDomCondition() calls in simplifySelectInst() and simplifyBinaryIntrinsic(), because they are subsumed by this one.



- I verified the test, If remove the  isImpliedByDomCondition() calls in simplifySelectInst() , there are 5 failed cases.

  LLVM :: CodeGen/Thumb2/mve-memtp-branch.ll
  LLVM :: Transforms/AggressiveInstCombine/lower-table-based-cttz-basics.ll
  LLVM :: Transforms/InstSimplify/select-implied.ll
  LLVM :: Transforms/LoopUnroll/runtime-exit-phi-scev-invalidation.ll
  LLVM :: Transforms/LoopUnroll/runtime-loop-at-most-two-exits.ll



- then, If continue remove the call in simplifyBinaryIntrinsic(), there is a extra failed case.

  LLVM :: Transforms/InstCombine/unrecognized_three-way-comparison.ll



- some seems regression, and others improve, so it can't be removed simple, should take a more work to fix them base the vesion of remove 2 isImpliedByDomCondition() ?

F26590712: image.png <https://reviews.llvm.org/F26590712>



================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:3918-3919
+      isImpliedByDomCondition(Pred, LHS, RHS, Q.CxtI, Q.DL);
+  if (Checked && *Checked == true)
+    return getTrue(ITy);
+
----------------
erikdesjardins wrote:
> This can be:
> 
> ```
> if (Checked)
>   return *Checked ? getTrue(ITy) : getFalse(ITy);
> ```
> 
> since it's also useful to know that the dominating condition is false.
> 
> I expect you'll hit this case if you add a test withj the dominating icmp changed to uge and swap the branch targets
- The following case tranform, but it in fact swap the branch before this transform, so it is same to urem_with_dominating_condition, I don't sure this case should be saved ?
```
define i8 @urem_with_dominating_condition_false(i8 %x, i8 %n) {
start:
  %cond = icmp uge i8 %x, %n
  br i1 %cond, label %.bb1, label %.bb0 ; Swap the branch targets
.bb0:
  %add = add i8 %x, 1
  %out = urem i8 %add, %n
  ret i8 %out
.bb1:
  ret i8 0
}
```


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

https://reviews.llvm.org/D144248