Page MenuHomePhabricator

[Statepoints][ISEL] gc.relocate uniquification should be based on SDValue, not IR Value.
ClosedPublic

Authored by dantrushin on Sep 15 2020, 7:46 AM.

Details

Summary

When exporting statepoint results to virtual registers we try to avoid
generating exports for duplicated inputs. But we erroneously use
IR Value* to check if inputs are duplicated. Instead, we should use
SDValue, because even different IR values can get lowered to the same
SDValue.
I'm adding a (degenerate) test case which emphasizes importance of this
feature for invoke statepoints.
If we fail to export only unique values we will end up with something
like that:

%0 = STATEPOINT
%1 = COPY %0

landing_pad:

<use of %1>

And when exceptional path is taken, %1 is left uninitialized (COPY is never
execute).

Diff Detail

Event Timeline

dantrushin created this revision.Sep 15 2020, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2020, 7:46 AM
dantrushin requested review of this revision.Sep 15 2020, 7:46 AM
reames accepted this revision.EditedSep 15 2020, 9:34 AM

LGTM

However, I think this a workaround (and a useful optimization) not a true fix. What would happen if for some reason a COPY didn't get eliminated? (Imagine no duplicates, just a single pointer being relocated, with relocated value used down the exceptional path) I don't have a test case for this, but I'd advise digging into that possibility further. I'd need to study invoke lowering more closely to understand the risk.

(Just to be clear, it is a useful optimization and that's the basis for the LGTM.)

This revision is now accepted and ready to land.Sep 15 2020, 9:34 AM
This revision was landed with ongoing or failed builds.Sep 21 2020, 5:45 AM
This revision was automatically updated to reflect the committed changes.