Page MenuHomePhabricator

[LoopPeel] Turn incorrect assert into a check
ClosedPublic

Authored by mkazantsev on Mar 12 2020, 1:40 AM.

Details

Summary

This patch replaces incorret assert with a check. It asserts that if SCEV
cannot prove isKnownPredicate(A != B), then it should be able to prove
isKnownPredicate(A == B).

Both these fact may be not provable. It is shown in the provided test:

Could not prove: {-294,+,-2}<%bb1> != 0
Asserting: {-294,+,-2}<%bb1> == 0

Obviously, this SCEV is not equal to zero, but 0 is in its range so we cannot
also prove that it is not zero.

Instead of assert, we should be checking the required conditions explicitly.

Diff Detail

Event Timeline

mkazantsev created this revision.Mar 12 2020, 1:40 AM

@lebedev.ri I am not sure what your logic implied here; maybe instead of removal of assert this check should be a part of if for profitability reasons.

mkazantsev edited the summary of this revision. (Show Details)Mar 12 2020, 1:42 AM
mkazantsev edited the summary of this revision. (Show Details)Mar 12 2020, 1:51 AM
mkazantsev edited the summary of this revision. (Show Details)

@lebedev.ri I am not sure what your logic implied here; maybe instead of removal of assert this check should be a part of if for profitability reasons.

In review where this assert was introduced in one of the previous versions of it there was a check instead of assert (see line 261+: https://reviews.llvm.org/D69617?id=227953)
I believe doing this check makes sense.

As @fedor.sergeev is pointing out, that assert exists because in the review
we thought it was an invariant that doesn't make sense to check via an if(),
but instead just assert() it. Clearly, that is wrong, so let's go back to checking it.

mkazantsev retitled this revision from [LoopPeel] Remove incorrect assertion in countToEliminateCompares to [LoopPeel] Turn incorrect assert into a check.
mkazantsev edited the summary of this revision. (Show Details)

Updated the patch in accordance with comments.

lebedev.ri accepted this revision.Mar 12 2020, 2:35 AM

LG, thank you.

This revision is now accepted and ready to land.Mar 12 2020, 2:35 AM
fhahn accepted this revision.Mar 12 2020, 3:29 AM
fhahn added inline comments.Mar 12 2020, 3:33 AM
llvm/test/Transforms/LoopUnroll/wrong_assert_in_peeling.ll
78

are the undef here and in other places required for the test case? Otherwise I think it would be better to replace them with constants/regular values to make the test case more robust. Same for branch on constants.

This revision was automatically updated to reflect the committed changes.
mkazantsev marked an inline comment as done.Mar 12 2020, 4:00 AM
mkazantsev added inline comments.
llvm/test/Transforms/LoopUnroll/wrong_assert_in_peeling.ll
78

We have quite a lot tests using undef and afaik there is no problems with their robustness.

fhahn added inline comments.Mar 12 2020, 4:46 AM
llvm/test/Transforms/LoopUnroll/wrong_assert_in_peeling.ll
78

It is not a huge problem (and depends on case by case).

But this test contains plenty of presumably unnecessary (conditional) branches on constants/undef, presumably unnecessary calls to llvm.experimental.deoptimize, branches on compare with undef. IMO this makes the test case unnecessarily verbose (and harder to focus on the actual problem it tests for). Also lots of the sub/icmp instructions are at the risk of being completely folded away in the future, e.g. if loop unrolling cost modeling would exploit undef more.

IIUC to trigger the assertions, I think all that would be required is a loop nest where we try to check a condition involving an expression with the inner and outer induction variable with the property mentioned in the description.