Page MenuHomePhabricator

[gvn] CSE gc.relocates based on meaning, not spelling

Authored by reames on Mar 4 2021, 12:40 PM.



The last two operands to a gc.relocate represent indices into the associated gc.statepoint's gc bundle list. (Effectively, gc.relocates are projections from the gc.statepoints multiple return values.)

We can use this to recognize when two gc.relocates are equivalent (and can be CSEd), even when the indices are non-equal. This is particular useful when considering a chain of multiple statepoints as it lets us eliminate all duplicate gc.relocates in a single pass.

The meat of the change isn't actually in GVN. For some reason, we had been marking gc.relocates as reading memory. This change undoes that. I believe this to be a legacy of very early implementation conservatism, but a skeptical view is appreciated. (The EarlyCSE change is simply moving the special casing from readonly to readnone handling.)

Diff Detail

Event Timeline

reames created this revision.Mar 4 2021, 12:40 PM
reames requested review of this revision.Mar 4 2021, 12:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2021, 12:40 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript
skatkov accepted this revision.Mar 5 2021, 1:41 AM

LGTM but please separate GVN and EarlyCSE changes.

This revision is now accepted and ready to land.Mar 5 2021, 1:41 AM
This revision was landed with ongoing or failed builds.Mar 5 2021, 10:20 AM
This revision was automatically updated to reflect the committed changes.

A bit of follow up here. This was reverted in cfe8f8e0f0 because the readnone changes exposed a couple of problems.

First, Serguei hit a couple of bugs in the lowering for statepoints which code movement exposed. These were fixed by D98324, and D98393 respectively. Both issues could in theory have occurred without the readnone change, but became much more common afterwards.

Second, Serguei hit an issue w/LICM sinking gc.relocates and gc.results out of loops, creating phis of token type (which are illegal). I'm going to explore a fix for this once I more fully understand the issue, but in the meantime, Serguei reverted the original change as we seem to keep hitting more issues.

To be clear, I support Serguei's revert and think it's fully justified given the multiple issues found. We'll revisit and try to reapply once exposed issues have all been fixed.

FYI, I filed for tracking try two at this.