This is an archive of the discontinued LLVM Phabricator instance.

[ConstraintElimination] Handle equality predicates
ClosedPublic

Authored by antoniofrighetto on Jun 3 2023, 8:11 AM.

Details

Summary

Simplification of equality predicates is now supported by transferring equalities into inequalities. This is achieved by separately checking that both isConditionImplied(A >= B) and isConditionImplied(A <= B) hold.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2023, 8:11 AM
antoniofrighetto requested review of this revision.Jun 3 2023, 8:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2023, 8:11 AM
fhahn added a reviewer: nikic.Jun 4 2023, 2:48 AM

Thanks for the patch! Some comments inline

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
1002

The changes here are mostly to avoid having to duplicate the code to create True/false constants, and indepdent of the patch, right? Could you split them of into a separate NFC patch?

1007–1014

It would be nice to print the condition with the negated predicate here (as separate NFC patch)

1036

It would probably be cleaner to move the cod e to check if a constraint is implied to a new member function of ConstraintTy and not expose ::isEq publicly. This should be cleaner overall, especially thinking about adding support for NE as well in the future.

llvm/test/Transforms/ConstraintElimination/eq.ll
382

I think we need some additional tests here, including ones where we have something like a == b && b == c as facts and a + 1 == c +1 and various permutations (also checking that a + 1 == b is not simplified and so on).

It would also be good to have tests where we only know things like a >= b && b >= c and check that we do not simply *a == c` and so on.

Added a few more tests, one we check a == b simplified to false, when a + 1 == b is assumed, the other in which no simplification can be done when assuming a >= b && b >= c. Testing the transitivity of equality has been extended to check if a + 1 == c + 1 also holds (via a select, note the orthogonal missed optimization there).

Polished up a bit the code & encapsulate the logic for handling equality constraints into isEqImplied.

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
1002

(Opened a separate patch for it, D152143)

fhahn added inline comments.Jun 5 2023, 6:11 AM
llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
1002

Great, thanks! Can you rebase this patch on top of D152143?

1034

Thanks for the latest update! I think it should even go further and be something like ConstraintTy::isImpliedBy(CS), so that all logic for querying CSToUse.isConditionImplied is done there.

Thanks for the suggestion, rebased & the logic for querying CS.isConditionImplied has been moved into ::isImpliedBy. I’m not completely sure if this may be further ameliorated, considering that computing the negated still needs to be done separately.

fhahn added inline comments.Jun 7 2023, 1:12 PM
llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
108

Would it be sufficient to have None, ConditionTrue, ConditionFalse and check all in isImpliedBy?

We may do that, and eventually ending up with the following:

auto CondImpliedStatus = R.isImpliedBy(CSToUse);

if (CondImpliedStatus == ConditionImpliedTy::ConditionTrue) {
  Changed = ReplaceCmpWithConstant(Cmp, true);
} else if (CondImpliedStatus == ConditionImpliedTy::ConditionFalse) {
  Changed = ReplaceCmpWithConstant(Cmp, false);
}

// Compute the negated separately.
auto Negated = ConstraintSystem::negate(R.Coefficients);
auto IsNegatedImplied =
    !Negated.empty() && CSToUse.isConditionImplied(Negated);

if (IsNegatedImplied) {
  Changed = ReplaceCmpWithConstant(Cmp, false);
}

in checkAndReplaceCondition, as we would still need to compute the negated separately (reproducer-remarks-debug.ll seems to be complaining). Perhaps we slightly lose some of the clarity / self-documenting that comes with distinguishing between EqualityTrue, ConditionTrue (may assist in debugging), though, likewise, that shouldn't be really that much of a problem. Could that work?

fhahn added a comment.Jun 9 2023, 5:26 AM

in checkAndReplaceCondition, as we would still need to compute the negated separately (reproducer-remarks-debug.ll seems to be complaining). Perhaps we slightly lose some of the clarity / self-documenting that comes with distinguishing between EqualityTrue, ConditionTrue (may assist in debugging), though, likewise, that shouldn't be really that much of a problem. Could that work?

I am not sure why we would need to compute the negation separately? Wouldn't ConditionHoldFalse cover exactly the case where the negated condition holds?

Argh, there was a leftover in reproducer-remarks-debug.ll that I hadn't noticed immediately that led to test failure. Should be better now!

nikic added inline comments.Jun 9 2023, 7:55 AM
llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
129

Might be cleaner to return std::optional<bool>? The use-site then becomes:

if (auto ImpliedCondition = R.isImpliedBy(CSToUse))
  return ReplaceCmpWithConstant(Cmp, *ImpliedCondition);
fhahn added inline comments.Jun 9 2023, 12:04 PM
llvm/include/llvm/Analysis/ConstraintSystem.h
135

as follow-up, it would be good to use negateOrEqual here as well.

140

Could you add a doc-comment for the function?

148

Could you add a doc-comment for the function?

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
590

I think it would probably be better to wrap the EQ handling in IsEq and then run the general code afterwards.

llvm/test/Transforms/ConstraintElimination/assumes.ll
636 ↗(On Diff #530037)

I think those tests would fit better in eq.ll as the assume here is only used to add known facts.

llvm/test/Transforms/ConstraintElimination/eq.ll
413

simpler to either use xor or and instead of the more complicated select.

llvm/test/Transforms/ConstraintElimination/reproducer-remarks-debug.ll
12

What changed here?

Thanks for the comments, should be now addressed.

llvm/test/Transforms/ConstraintElimination/reproducer-remarks-debug.ll
12

There is a leftover (---) for the current implementation, as even when IsConditionImplied is true, isConditionImplied(Negated) is computed, leading to the following debug output:

---
%a + -1 * null <= 0
%a + -1 * null <= 0
sat

This doesn't occur anymore now, as, for the general case, we do not compute IsNegatedImplied when IsConditionImplied is true.

fhahn accepted this revision.Jun 11 2023, 2:45 AM

LGTM with the additional nits addressed, thanks!

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
130

nit: Would be good to also document the return value: std;:nullopt if iy couldn't prove the constraint is true or false, otherwise returns whether the constraint was prove true or false.

583

Move this block and the next one after

if (IsConditionImplied && IsNegatedOrEqualImplied)
  return true;

to avoid unnecessary queries.

This revision is now accepted and ready to land.Jun 11 2023, 2:45 AM