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

[Yingwei Zheng via Phabricator via llvm-commits]

dtcxzyw added a comment.

In D155412#4523259 <https://reviews.llvm.org/D155412#4523259>, @fhahn wrote:

> LGTM, thanks! Please make sure there also are tests that use mixed signed & unsigned predicates as per @nikic's comment. I could't spot one in the latest version but maybe I missed it.



> I think you're current missing a test for mixing signed and unsigned predicates.

in function `smin_ule_mixed` at line 255

> 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.

in function `smin_branchless` at line 319



================
Comment at: llvm/test/Transforms/ConstraintElimination/minmax.ll:470
 declare i32 @llvm.umin.i32(i32, i32)
 declare i32 @llvm.umax.i32(i32, i32)
----------------
nikic wrote:
> 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.
> 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