This is an archive of the discontinued LLVM Phabricator instance.

Differential D38765

Don't replace constants with constants in GVN
ClosedPublic

Authored by alexcrichton on Oct 10 2017, 2:48 PM.

Details

Reviewers
• dberlin
Commits
rGe2138fe41b8c: [GVN] Don't replace constants with constants.
rL315429: [GVN] Don't replace constants with constants.
Summary

This is a patch to fix bug 34908 which avoids an assertion being tripped when an assume intrinsic is called on a constant value.

Diff Detail

Event Timeline

alexcrichton created this revision.Oct 10 2017, 2:48 PM
efriedma added subscribers: • dberlin, llvm-commits.Oct 10 2017, 5:52 PM
• dberlin added a comment.Oct 10 2017, 6:00 PM

Suggesting a slightly different fix.

lib/Transforms/Scalar/GVN.cpp
1368

At this point, if V is a constant, you should return false.

There is nothing useful we can do:
There is no equality it is really propagating, since it's not a comparison, and we've already taken care of the case where we have assume(false).

Every other constant is equivalent to assume(true).

That leaves two options:

  1. Delete the intrinsic, as it is now pointless.
  2. Do nothing here and return false.

Either seems reasonable.

As an aside, it's mentioned this came from a rust issue.
It's probably worth looking to see if something propagated this constant *into* the assume (rather than it being generated by the frontend as an assume of a constant)

If so, *that* should be filed as a bug, as that propagation lost valuable information.

alexcrichton updated this revision to Diff 118531.Oct 10 2017, 8:39 PM

Sure thing, updated!

Unfortunately the original Rust issue is sort of hard to reproduce. It only came up during ThinLTO passes which I didn't find easy to run from the LLVM CLI tools, but I can always provide the original IR (it's quite large) if desired!

davide added inline comments.Oct 10 2017, 8:47 PM
test/Transforms/GVN/no-crash-on-undef-assume.ll
1

check the output?

alexcrichton marked an inline comment as done.Oct 10 2017, 8:48 PM
alexcrichton added inline comments.
test/Transforms/GVN/no-crash-on-undef-assume.ll
1

Oh I wasn't quite sure what this should be checking, this is basically just asserting it doesn't crash (today this test case crashes w/ asserts enabled)

davide added inline comments.Oct 10 2017, 8:52 PM
test/Transforms/GVN/no-crash-on-undef-assume.ll
1

you could probably just pipe the output to FileCheck %s and insert some check lines (please see any other test in tree).
The reason is twofold:

  1. if some xform changes this pattern, then we notice (imagine, e.g., at some point we decide to not strip calls to assume [I think this is what happens now])
  2. you could replace opt with /bin/true and the test will still pass (while checking the output with filecheck makes this test more robust.

Thanks!

• dberlin added inline comments.Oct 10 2017, 8:56 PM
test/Transforms/GVN/no-crash-on-undef-assume.ll
1

Yeah, pipe it to filecheck.
If you do that, you can use update_test_checks.py and it'll make CHECK lines for you.

For example, to update newgvn tests, i usually use:
python ../../../utils/update_test_checks.py --opt-binary=../../../debug-build/bin/opt <filename.ll>

from the tests/Transforms/NewGVN directory

• dberlin accepted this revision.Oct 10 2017, 8:58 PM

Okay if you address the remaining comments.
Thanks!

lib/Transforms/Scalar/GVN.cpp
1365

Can you add a comment saying "if it's not false, and constant, it must evaluate to true. This means our assume is assume(true), and thus, pointless, and we don't want to do anything more here"

or something similar.

This revision is now accepted and ready to land.Oct 10 2017, 8:58 PM
alexcrichton updated this revision to Diff 118534.Oct 10 2017, 9:02 PM

Ok thanks for the help! I've updated with some CHECK directives now. Mind helping me land the patch as well?

davide added a comment.Oct 10 2017, 9:08 PM

LGTM once you address last Danny's concern (i.e. adding a comment :)
Alex, do you have commit access or you want me to commit that on your behalf? Thanks!

alexcrichton updated this revision to Diff 118535.Oct 10 2017, 9:10 PM
alexcrichton marked 5 inline comments as done.

Oops sorry about that! Missed that last request, but should be taken care of now.

Also yeah if you could commit on my behalf that'd be great, thanks!

davide added a comment.Oct 10 2017, 9:12 PM

I'll do a round of testing on my machine and commit for you.

Thanks!

Closed by commit rL315429: [GVN] Don't replace constants with constants. (authored by davide). · Explain WhyOct 10 2017, 9:22 PM
This revision was automatically updated to reflect the committed changes.
davide added a comment.Oct 10 2017, 9:22 PM

In trunk now.

 git svn dcommit --interactive
Committing to https://llvm.org/svn/llvm-project/llvm/trunk ...
commit 9bafdc0fb7038b1729d180b7306bae1f068c7785
Author: Davide Italiano <dccitaliano@gmail.com>
Date:   Tue Oct 10 21:20:54 2017 -0700

    [GVN] Don't replace constants with constants.

    This fixes PR34908. Patch by Alex Crichton!

    Differential Revision:  https://reviews.llvm.org/D38765

 create mode 100644 test/Transforms/GVN/pr34908.ll
Commit this patch to SVN? ([y]es (default)|[n]o|[q]uit|[a]ll): y
        A       test/Transforms/GVN/pr34908.ll
        M       lib/Transforms/Scalar/GVN.cpp
Committed r315429

Revision Contents

PathSize
lib/
Transforms/
Scalar/
2 lines
test/
Transforms/
GVN/
8 lines

Diff 118531

lib/Transforms/Scalar/GVN.cpp

Show First 20 Lines • Show All 1,356 Lines • ▼ Show 20 Lines if (Cond->isZero()) {
// this code is not reachable. FIXME: We could insert unreachable // this code is not reachable. FIXME: We could insert unreachable
// instruction directly because we can modify the CFG. // instruction directly because we can modify the CFG.
new StoreInst(UndefValue::get(Int8Ty), new StoreInst(UndefValue::get(Int8Ty),
Constant::getNullValue(Int8Ty->getPointerTo()), Constant::getNullValue(Int8Ty->getPointerTo()),
IntrinsicI); IntrinsicI);
} }
markInstructionForDeletion(IntrinsicI); markInstructionForDeletion(IntrinsicI);
return false; return false;
} else if (isa<Constant>(V)) {
dberlinUnsubmitted
Done
ReplyInline Actions

Can you add a comment saying "if it's not false, and constant, it must evaluate to true. This means our assume is assume(true), and thus, pointless, and we don't want to do anything more here"

or something similar.

dberlin: Can you add a comment saying "if it's not false, and constant, it must evaluate to true. This…
return false;
} }
dberlinUnsubmitted
Done
ReplyInline Actions

At this point, if V is a constant, you should return false.

There is nothing useful we can do:
There is no equality it is really propagating, since it's not a comparison, and we've already taken care of the case where we have assume(false).

Every other constant is equivalent to assume(true).

That leaves two options:

  1. Delete the intrinsic, as it is now pointless.
  2. Do nothing here and return false.

Either seems reasonable.

As an aside, it's mentioned this came from a rust issue.
It's probably worth looking to see if something propagated this constant *into* the assume (rather than it being generated by the frontend as an assume of a constant)

If so, *that* should be filed as a bug, as that propagation lost valuable information.

dberlin: At this point, if V is a constant, you should return false. There is nothing useful we can do…
Constant *True = ConstantInt::getTrue(V->getContext()); Constant *True = ConstantInt::getTrue(V->getContext());
bool Changed = false; bool Changed = false;
for (BasicBlock *Successor : successors(IntrinsicI->getParent())) { for (BasicBlock *Successor : successors(IntrinsicI->getParent())) {
BasicBlockEdge Edge(IntrinsicI->getParent(), Successor); BasicBlockEdge Edge(IntrinsicI->getParent(), Successor);
// This property is only true in dominated successors, propagateEquality // This property is only true in dominated successors, propagateEquality
// will check dominance for us. // will check dominance for us.
▲ Show 20 Lines • Show All 1,168 Lines • Show Last 20 Lines

test/Transforms/GVN/no-crash-on-undef-assume.ll

  • This file was added.
; RUN: opt < %s -gvn -S
davideUnsubmitted
Done
ReplyInline Actions

check the output?

davide: check the output?
alexcrichtonAuthorUnsubmitted
Done
ReplyInline Actions

Oh I wasn't quite sure what this should be checking, this is basically just asserting it doesn't crash (today this test case crashes w/ asserts enabled)

alexcrichton: Oh I wasn't quite sure what this should be checking, this is basically just asserting it…
davideUnsubmitted
Done
ReplyInline Actions

you could probably just pipe the output to FileCheck %s and insert some check lines (please see any other test in tree).
The reason is twofold:

  1. if some xform changes this pattern, then we notice (imagine, e.g., at some point we decide to not strip calls to assume [I think this is what happens now])
  2. you could replace opt with /bin/true and the test will still pass (while checking the output with filecheck makes this test more robust.

Thanks!

davide: you could probably just pipe the output to `FileCheck %s` and insert some check lines (please…
dberlinUnsubmitted
Done
ReplyInline Actions

Yeah, pipe it to filecheck.
If you do that, you can use update_test_checks.py and it'll make CHECK lines for you.

For example, to update newgvn tests, i usually use:
python ../../../utils/update_test_checks.py --opt-binary=../../../debug-build/bin/opt <filename.ll>

from the tests/Transforms/NewGVN directory

dberlin: Yeah, pipe it to filecheck. If you do that, you can use update_test_checks.py and it'll make…
define i1 @foo() {
call void @llvm.assume(i1 undef)
ret i1 undef
}
declare void @llvm.assume(i1)