Page MenuHomePhabricator

[wip][mips] Use AltOrders to prevent using odd FP-registers
ClosedPublic

Authored by atanasyan on Mar 25 2019, 2:28 PM.

Details

Summary

To disable using of odd floating-point registers (O32 ABI and -mno-odd-spreg command line option) such registers and their super-registers added to the set of reserved registers. In general, it works. But there is at least one problem - in case of enabled machine verifier pass some floating-point tests failed because live ranges of register units that are reserved is not empty and verification pass failed with "Live segment doesn't end at a valid instruction" error message.

There is D35985 patch which tries to solve the problem by explicit removing of register units. This solution did not get approval.

I would like to use another approach for prevent using odd floating-point registers - define AltOrders and AltOrderSelect for MIPS floating point register classes. Such AltOrders contains reduced set of registers. At first glance, such solution does not break any test cases and allows enabling machine instruction verification for all MIPS test cases.

Does anybody see any problems on this way?

Diff Detail

Repository
rL LLVM

Event Timeline

atanasyan created this revision.Mar 25 2019, 2:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2019, 2:28 PM

I don't see other reason for registers form OddSPRegClass to be declared as as Reserved other then to affect Allocation order in register allocation.
From description of nooddspreg : "Disable odd numbered single-precision registers". I would say that we only need to make sure that odd float registers can't be used during register allocation that is to affect Allocation order in some way to make sure odd registers don't appear there. I saw that similar problem for msa was handled with use of different register class (MSA128WRegClass or MSA128WEvensRegClass) in MipsSETargetLowering::emitCOPY_FW.
Here this is done with AltOrders without having to define registers from OddSPRegClass as Reserved. MipsGenRegisterInfo.inc also got smaller, everything looks good from my side.

What I don't really understand purpose of the affected test, it forces use of $f13 with asm instruction despite the fact it is disabled with +nooddspreg option. Shouldn't we just report an error that odd register can't be used with +nooddspreg?

My initial comments are that this looks ok, but I would like a longer look at it.

llvm/lib/Target/Mips/MipsRegisterInfo.td
385 ↗(On Diff #192206)

Nit: Can you provide a comment summarising the reason for the AltOrder additions?

394 ↗(On Diff #192206)

To the best of my knowledge, the FGRH32 regclass is unused. I did remove it locally and run the test-suite, and as best I can recall it made no difference but I didn't run all configurations.

Dealing with that would be a separate commit.

414 ↗(On Diff #192206)

Nit: Can you provide a comment summarising the reason for the AltOrder additions?

atanasyan updated this revision to Diff 192526.EditedMar 27 2019, 3:27 PM
  • Use more compact definitions of AltOrders
  • Add comments
  • Thanks for pointing to the MSA128WEvensRegClass. I'm going to investigate a possibility to do the same trick with AltOrders in this case. If it's impossible, find the reason.
  • Both GNU assembler and LLVM show warning if odd floating-point register appears in an asm file. As far as I understand the -mno-odd-spreg prevents using odd registers by backend, but does not prevent inserting such registers into asm file directly by a user.
  • Agreed, FGRH32 looks redundant.

Both GNU assembler and LLVM show warning if odd floating-point register appears in an asm file. As far as I understand the -mno-odd-spreg prevents using odd registers by backend, but does not prevent inserting such registers into asm file directly by a user.

Thanks for clarification.

llvm/lib/Target/Mips/MipsRegisterInfo.td
387 ↗(On Diff #192526)

let AltOrders = [(decimate FGR32, 2)]; also works here.

sdardis accepted this revision.Mar 28 2019, 3:17 PM

I think this is a viable approach for this issue. Based on the dumps from -debug I can see that the backend should only be selecting even register for FGR32.

I'd suggest dropping the WIP tag and waiting a day or two to see if @dsanders / @Petar.Avramovic / llvm-commits has any other suggestions.

LGTM.

This revision is now accepted and ready to land.Mar 28 2019, 3:17 PM

Thanks for review.

This revision was automatically updated to reflect the committed changes.