Page MenuHomePhabricator

[GVN] Try to be more principled about non-integral pointers
Needs ReviewPublic

Authored by vtjnash on Mar 21 2019, 12:03 PM.



Very similar to the recent work, such as "Fix a crash bug w/non-integral pointers and memtransfers". But try to be more precise about what should and should not be permitted, and consolidate all users into calling canCoerceMustAliasedValueToLoad (since that is the predicate that VNCoercion will check). Similarly, teach the ConstantFolding analysis pass that's it's not legal to replace a load of a bitcast constant (having a non-integral addrspace) with a bitcast of the value of that constant (with a different non-integral addrspace).

Also simplify some of the negative tests for transforms by avoiding a type change in their bitcast, and add some positive versions of the same tests, to test that they otherwise should work. We need to also fix a globalopt test, since after this change, LLVM is able to realize that that test actually is a valid transform (NULL is always a known bit-pattern) and doesn't emit the failure remarks.

Diff Detail

Event Timeline

vtjnash created this revision.Mar 21 2019, 12:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2019, 12:03 PM
reames requested changes to this revision.Mar 22 2019, 3:11 PM

I really like what you're trying to do here, but the actual change is problematic. You're mixing non-functional refactoring with semantic change which makes this really hard to understand. Please split this into at least two parts. (I say at least because you appear to have some code motion changes which could be landed trivially without review as well.)

This revision now requires changes to proceed.Mar 22 2019, 3:11 PM
vtjnash updated this revision to Diff 191979.Mar 22 2019, 5:59 PM
vtjnash edited the summary of this revision. (Show Details)

Split out part of the changes into two parent commits.

Thanks @reames, I appreciate what you've already done here. I had started this a long time ago, where more of it was a functional change, then discovered you had recently already made most of the essential fixes and written the necessary tests. I've changed this into a stack of 3 commits that touch slightly different areas.

reames requested changes to this revision.Apr 1 2019, 3:27 PM

Waiting for previous patches to land before reviewing.

This revision now requires changes to proceed.Apr 1 2019, 3:27 PM
vtjnash updated this revision to Diff 196653.Apr 25 2019, 9:21 AM