Page MenuHomePhabricator

[GVN] Try to be more careful about handling constant zero and SVE
AbandonedPublic

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

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

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

330 ↗(On Diff #196653)

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.Jul 13 2020, 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.
reames requested changes to this revision.Oct 15 2020, 10:56 AM
reames added inline comments.
llvm/lib/Transforms/Scalar/GVN.cpp
1037

Comment change LGTM if you want to land this separately. (Please do.)

llvm/lib/Transforms/Utils/VNCoercion.cpp
21

This looks on the surface to be a bug that needs fixed in the caller. Maybe there's a subtly I'm missing here though?

33

Given we know this *can't* be a scalable type, the generalization to type size doesn't seem to add any value here?

59

It's not really clear to me what value you get from splitting out the type based vs value based variants. You duplicate code and there doesn't seem to be any benefit (in this patch). I am tempted to reject this change.

445

This looks odd. How can this be NFC if you're actually changing the generated code?

I suspect that your patch description is stale and needs updated. If so, you're also missing tests.

This revision now requires changes to proceed.Oct 15 2020, 10:56 AM
vtjnash retitled this revision from [GVN] Try to be more careful about handling constant zero, NFCI to [GVN] Try to be more careful about handling constant zero and SVE.Oct 15 2020, 11:20 AM

I've removed NFCI, since I ended up changing some of the SVE handling when attempting to rebase it

vtjnash abandoned this revision.Nov 3 2020, 10:06 AM