This is an archive of the discontinued LLVM Phabricator instance.

[SimpleLoopUnswitch] Don't unswitch constant conditions
ClosedPublic

Authored by DaniilSuchkov on Sep 29 2021, 12:10 PM.

Details

Summary

Added an additional check for constants after simplification of select _, true, false pattern. We need to prevent attempts to unswitch constant conditions for two reasons:
a) Doing that doesn't make any sense, in the best case it will just burn some compile time.
b) SimpleLoopUnswitch isn't designed to unswitch constant conditions (due to (a)), so attempting that can cause miscompiles. The attached testcase is an example of such miscompile.

Also I added an assertion that'll make sure we aren't trying to replace constants, so it will help us prevent such bugs in future. The assertion from D110751 is another layer of protection against such cases.

Diff Detail

Event Timeline

DaniilSuchkov created this revision.Sep 29 2021, 12:10 PM
DaniilSuchkov requested review of this revision.Sep 29 2021, 12:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2021, 12:10 PM

can you add the motivation for this change to the description?

DaniilSuchkov edited the summary of this revision. (Show Details)Sep 29 2021, 3:51 PM

can you add the motivation for this change to the description?

Does it look good now? Or you think something else should be added?

thanks, the description is good, but the test doesn't seem right

llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-invariant-select-bug.ll
9

this doesn't actually happen right now?

DaniilSuchkov added inline comments.Sep 30 2021, 11:46 AM
llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-invariant-select-bug.ll
9

Right now, since neither this patch, nor D110751 are landed, that's exactly what happens. Sorry, I forgot to add D110751 as a dependency. With that patch we get an assertion failure in this case instead of what you see in CHECKs.

Is this falling into the traditional trap of the fact that constants and constant expressions are both represented by the Constant class?
Presumably while we obviously shouldn't unswitch immediate constants, constant expressions are a fair game still?

Is this falling into the traditional trap of the fact that constants and constant expressions are both represented by the Constant class?
Presumably while we obviously shouldn't unswitch immediate constants, constant expressions are a fair game still?

I don't quite understand your question. Let me explain what's going on.

This bug is introduced by this quick fix: https://reviews.llvm.org/rG6b4b1dc6ec6f
Before that we would never add a constant (i.e. Value with isa<Constant> == true) to the list of unswitch candidates. After that patch it became possible for true or false to become a condition that we're trying to unswitch.

As for the assertion which I added, in replaceLoopInvariantUses there's a piece of code that does very similar thing and it has assert(!isa<Constant>(Invariant) && "Why are we unswitching on a constant?");, so lack of such an assertion in this code seems just an oversight.

DaniilSuchkov updated this revision to Diff 376326.EditedSep 30 2021, 12:27 PM
DaniilSuchkov edited the summary of this revision. (Show Details)

Rebased on top of new version of D110751. Now it should be easier to understand what's going on with the test.

rebasing this on D110751 is confusing, especially since the test is XFAIL in that patch
I'd just add the test and run update_test_checks.py on it in this patch

then just add the asserts in D110751 in don't touch any test cases in that patch

rebasing this on D110751 is confusing, especially since the test is XFAIL in that patch
I'd just add the test and run update_test_checks.py on it in this patch

then just add the asserts in D110751 in don't touch any test cases in that patch

Well, D110751 turns the miscompile into and assertion failure, and with this patch instead of assertion failure we'll get a completely valid result.
I think this sequence of changes makes sense.

rebasing this on D110751 is confusing, especially since the test is XFAIL in that patch
I'd just add the test and run update_test_checks.py on it in this patch

then just add the asserts in D110751 in don't touch any test cases in that patch

Well, D110751 turns the miscompile into and assertion failure, and with this patch instead of assertion failure we'll get a completely valid result.
I think this sequence of changes makes sense.

Probably a matter a preference, but I think we should fix the issue first. I'm not a fan of XFAIL tests. You could even merge the two patches into one.
Any other preferences from other reviewers?

nikic added a subscriber: nikic.Sep 30 2021, 3:11 PM

rebasing this on D110751 is confusing, especially since the test is XFAIL in that patch
I'd just add the test and run update_test_checks.py on it in this patch

then just add the asserts in D110751 in don't touch any test cases in that patch

Well, D110751 turns the miscompile into and assertion failure, and with this patch instead of assertion failure we'll get a completely valid result.
I think this sequence of changes makes sense.

Probably a matter a preference, but I think we should fix the issue first. I'm not a fan of XFAIL tests. You could even merge the two patches into one.
Any other preferences from other reviewers?

Agree, we should fix the issue first and then land the assert. We shouldn't add asserts that are known to fail in advance.

DaniilSuchkov edited the summary of this revision. (Show Details)

Now D110751 is on top of this change.

aeubanks added inline comments.Sep 30 2021, 3:32 PM
llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-invariant-select-bug.ll
9

where is this diff coming from? the test isn't in tree
could just add this test in this patch

(otherwise this patch lg)

DaniilSuchkov added inline comments.Sep 30 2021, 3:38 PM
llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-invariant-select-bug.ll
9

I'll land this test with the patch (in a separate commit). I prefer to add tests separately because otherwise reviewers have to guess how the patch is connected with the test.

aeubanks accepted this revision.Sep 30 2021, 9:10 PM

lg, thanks!

This revision is now accepted and ready to land.Sep 30 2021, 9:10 PM