[PATCH] D155412: [ConstraintElim] Add facts implied by MinMaxIntrinsic
Nikita Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 21 03:28:11 PDT 2023
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
More information about the llvm-commits
mailing list