This is an archive of the discontinued LLVM Phabricator instance.

[MachinePipeliner] Handle failing constrainRegClass
ClosedPublic

Authored by dmgreen on Jun 16 2022, 7:45 AM.

Details

Summary

The included test hits a verifier problems as one of the instructions:

%113:tgpreven, %114:tgprodd = MVE_VMLSLDAVas16 %12:tgpreven(tied-def 0), %11:tgprodd(tied-def 1), %7:mqpr, %8:mqpr, 0, $noreg, $noreg

Has two inputs that come from different PHIs with the same base reg, but conflicting regclasses:

%11:tgprodd = PHI %103:gpr, %bb.1, %16:gpr, %bb.2
%12:tgpreven = PHI %103:gpr, %bb.1, %17:gpr, %bb.2

The MachinePipeliner would attempt to use %103 for both the %11 and %12 operands in the prolog, constraining the register class to the common subset of both. Unfortunately there are no registers that are both odd and even, so the second constrainRegClass fails. Fix this situation by inserting a COPY for the second if the call to constrainRegClass fails.

The register allocation can then fold that extra copy away. The register allocation of Q regs changed with this test, but the R regs were the same and no new instructions are needed in the final assembly.

Diff Detail

Event Timeline

dmgreen created this revision.Jun 16 2022, 7:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2022, 7:45 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
dmgreen requested review of this revision.Jun 16 2022, 7:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2022, 7:45 AM

Looks good to me. I assume that the test case can't be reduced while retaining the behaviour.

samtebbs accepted this revision.Jun 16 2022, 7:56 AM
This revision is now accepted and ready to land.Jun 16 2022, 7:56 AM
bcahoon accepted this revision.Jun 16 2022, 4:17 PM

Thanks for making this change. It's an interesting case. We handled a related issue, with subregisters, by adding code to preprocessPhiNodes to create copies. I don't have a problem with approach in this patch. There may be other cases in this code that need a similar change? Not sure...

Thanks for making this change. It's an interesting case. We handled a related issue, with subregisters, by adding code to preprocessPhiNodes to create copies. I don't have a problem with approach in this patch. There may be other cases in this code that need a similar change? Not sure...

Thanks for the suggestions. I've looked through and checked the other uses of constrainRegClass, changing them to asserts so that it is more obvious where the call fails.

This revision was landed with ongoing or failed builds.Jun 19 2022, 10:55 AM
This revision was automatically updated to reflect the committed changes.