[PATCH] D155412: [ConstraintElim] Add facts implied by MinMaxIntrinsic

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Implementation looks good to me.



================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1380
       Info.addFact(Pred, A, B, CB.NumIn, CB.NumOut, DFSInStack);
-      if (ReproducerModule && DFSInStack.size() > ReproducerCondStack.size())
+      if (ReproducerModule && DFSInStack.size() > ReproducerCondStack.size()) {
         ReproducerCondStack.emplace_back(Pred, A, B);
----------------
Unnecessary braces


================
Comment at: llvm/test/Transforms/ConstraintElimination/minmax.ll:29
 
 define i1 @umax_ugt_uge(i32 %x, i32 %y) {
 ; CHECK-LABEL: define i1 @umax_ugt_uge
----------------
These tests could be more compact by testing both ugt and uge inside the if. The standard pattern in ConstraintElim tests seems to be to combine multiple conditions with `xor i1`.


================
Comment at: llvm/test/Transforms/ConstraintElimination/minmax.ll:470
 declare i32 @llvm.umin.i32(i32, i32)
 declare i32 @llvm.umax.i32(i32, i32)
----------------
I think you're current missing a test for mixing signed and unsigned predicates.

I'd also suggesting to test something like `x pred min(x, y)`, where there is no branch involved, and you're just directly using the fact implied by the min/max.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155412