Simplification of non-equality predicates for solving constraints system is now supported by checking the validity of related inequalities and equalities.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Transforms/Scalar/ConstraintElimination.cpp | ||
---|---|---|
536 | I think this is not correct, as this effectively means we will be adding Op0 <= Op1 to the constraint system as fact for Op0 != Op1 at the moment. To start with, it would be good to only support NE predicates for solving, not when adding facts. A test case that shows the issue is below. Would be good to have similar variants with UGE/UGT/ULT and signed variants. define i1 @assume(i64 %a, i64 %b, i64 %c) { %1 = icmp ne i64 %a, %b tail call void @llvm.assume(i1 %1) %r = icmp ule i64 %a, %b ret i1 %r } | |
llvm/test/Transforms/ConstraintElimination/ne.ll | ||
298 | please add the additional test coverage separately. It would also be good to use names for the various values (instead of numbers) so they are easier to modify/play with. |
Reviewing this, as, indeed, I have been meaning to add this for solving only, not as fact for now, which would need further handling. I believe that simplifying NE to ULE for solving only, as it is being done for EQ, should be OK though. For facts, we should add something close to (Op0 < Op1) ∨ (Op1 > Op0), but I need to think further on how to model this. Perhaps, for now we could return from addFact when if (!R.isValid(*this) || R.IsNe()).
llvm/lib/Transforms/Scalar/ConstraintElimination.cpp | ||
---|---|---|
671–673 | might be good to expand the comment here, explaining how this handles == and !=. Same below. | |
llvm/test/Transforms/ConstraintElimination/assumes.ll | ||
626 ↗ | (On Diff #530510) | better move those to ne.ll and also add variants with ule/uge. Would be good to add the tests separately. |
I think for now it should be fine to handle ICMP_NE in solving, support for facts to be added at a later time.
LGTM, thanks!
llvm/test/Transforms/ConstraintElimination/ne.ll | ||
---|---|---|
408 | Might be good to add a TODO for signed support. |
I think this is not correct, as this effectively means we will be adding Op0 <= Op1 to the constraint system as fact for Op0 != Op1 at the moment. To start with, it would be good to only support NE predicates for solving, not when adding facts.
A test case that shows the issue is below. Would be good to have similar variants with UGE/UGT/ULT and signed variants.