This is an archive of the discontinued LLVM Phabricator instance.

Don't replace constants with constants in GVN
ClosedPublic

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

Details

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

Repository
rL LLVM

Event Timeline

alexcrichton created this revision.Oct 10 2017, 2:48 PM

Suggesting a slightly different fix.

lib/Transforms/Scalar/GVN.cpp
1366 ↗(On Diff #118484)

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.

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 ↗(On Diff #118531)

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 ↗(On Diff #118531)

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 ↗(On Diff #118531)

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 ↗(On Diff #118531)

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 ↗(On Diff #118531)

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

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

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 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!

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

Thanks!

This revision was automatically updated to reflect the committed changes.

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