This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Use MQPR not QPR for MVE registers
ClosedPublic

Authored by dmgreen on Aug 14 2019, 4:55 AM.

Details

Summary

We should be using MQPR, and if we don't we can get COPYs and PHIs created for QPR. These get folded into instructions, failing verification checks.

Diff Detail

Repository
rL LLVM

Event Timeline

dmgreen created this revision.Aug 14 2019, 4:55 AM
ostannard added inline comments.Aug 14 2019, 6:04 AM
llvm/test/CodeGen/Thumb2/mve-crash-qpr.ll
10 ↗(On Diff #215083)

I don't think these check lines are really adding any value, this is a complex test file so we'd expect the generated code to change as we improve the MVE code generation. Could we instead test this by stopping as soon as possible after instruction selection, and checking that the vregs are all MQPR class in the MIR? That would also allow the IR to be greatly reduced.

dmgreen updated this revision to Diff 215883.Aug 19 2019, 6:48 AM
dmgreen marked an inline comment as done.

Sorry for the delay. This test case was originally dependant upon masked stores. I've redone it without that and simplified it a little.

llvm/test/CodeGen/Thumb2/mve-crash-qpr.ll
10 ↗(On Diff #215083)

I'm not a huge fan of weird tests. I'd prefer it if they were all pretty boring, just simple enough to be autogenerated. They are easy to update if they do change, and you can see if they are smaller or the same. They act as useful codegen checks, to see when changes do have an effect on codegen. Perhaps this test isn't the best example of that though.

I've tried to cut it down and removed most of the check lines. Hopefully its simple enough now.

dmgreen updated this revision to Diff 216410.EditedAug 21 2019, 9:17 AM

Found another one. This time in getLargestLegalSuperClass.

dmgreen updated this revision to Diff 216440.Aug 21 2019, 11:29 AM

Updated check lines

ostannard accepted this revision.Sep 2 2019, 3:56 AM

LGTM, thanks.

This revision is now accepted and ready to land.Sep 2 2019, 3:56 AM
This revision was automatically updated to reflect the committed changes.