Page MenuHomePhabricator

[GVN] Try to be more careful about handling constant zero, NFCI
Needs ReviewPublic

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

Details

Reviewers
reames
Summary

Casting a zero should always be permitted. Make sure the canCoerce check reflects that, and that all users explicitly handle it.

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

LGTM specifically to the noted parts only. Remainder to be rebased on landed parts and reviewed after a week or so of burn confirms belief named parts are NFC.

lib/Transforms/Utils/VNCoercion.cpp
49

Consistency: we should probably be avoiding mmx here based on constant folding logic and langref, but given the original code being restructured doesn't handle this, I'm okay with this being done as followup.

52

I'm okay with the above split landing as a NFCI, then reviewing the rest.

330

This are grouped with the split above.

vtjnash updated this revision to Diff 228092.Nov 6 2019, 10:13 AM
vtjnash edited the summary of this revision. (Show Details)

Split commit into two pieces, each more clearly NFCI

vtjnash retitled this revision from [GVN] Try to be more principled about non-integral pointers to [GVN] Try to be more permissive with non-integral pointers zeros [NFCI].Nov 6 2019, 10:15 AM
vtjnash edited the summary of this revision. (Show Details)
vtjnash updated this revision to Diff 277659.Mon, Jul 13, 10:06 PM
vtjnash retitled this revision from [GVN] Try to be more permissive with non-integral pointers zeros [NFCI] to [GVN] Try to be more careful about handling constant zero, NFCI.