This is an archive of the discontinued LLVM Phabricator instance.

[mips] Add support for Virtualization ASE
ClosedPublic

Authored by vstefanovic on Mar 26 2018, 12:59 PM.

Details

Summary

Instructions: tlbginv, tlbginvf, tlbgp, tlbgr, tlbgwi, tlbgwr, hypcall

mfgc0, mtgc0, mfhgc0, mthgc0, dmfgc0, dmtgc0,

Assembler directives: .set virt, .set novirt,

.module virt, .module novirt

Attribute: virt
.MIPS.abiflags: VZ (0x100)

Diff Detail

Repository
rL LLVM

Event Timeline

vstefanovic created this revision.Mar 26 2018, 12:59 PM
sdardis requested changes to this revision.Apr 4 2018, 4:25 AM

This looks mostly ok. The big changes required is that these instructions should go in the relevant base architecture .td, as I don't believe there's enough of them to warrant going into two separate files.

lib/Target/Mips/MicroMips32r6InstrFormats.td
879 ↗(On Diff #139841)

The virtualization standard for microMIPS says that the minimum ISA level for that ASE is microMIPS 5.0, so this class definition should go in lib/Target/Mips/MicroMipsInstrFormats.td.

Also, the 'MMR6' suffix should then be 'MM'.

lib/Target/Mips/MicroMips32r6InstrInfo.td
1410 ↗(On Diff #139841)

These lines are overly long.

lib/Target/Mips/Mips64InstrInfo.td
559 ↗(On Diff #139841)

Drop the 'Predicates = [HasMips64]' for this line as it overrides ISA_MIPS64, ASE_VIRT adjectives.

561 ↗(On Diff #139841)

This should be ISA_MIPS64R5, ASE_VIRT and likewise below.

885 ↗(On Diff #139841)

This should be ISA_MIPS64R5, ASE_VIRT and likewise below.

lib/Target/Mips/MipsEVAInstrFormats.td
55–62 ↗(On Diff #139841)

These instructions should go in lib/Target/Mips/MipsInstr{Info, Formats}.td. They are not associated with the EVA ASE.

lib/Target/Mips/MipsEVAInstrInfo.td
230–239 ↗(On Diff #139841)

These instructions should be guarded for MIPS32R5, not MIPS32.

lib/Target/Mips/MipsInstrInfo.td
2333–2336 ↗(On Diff #139841)

These instructions should be guarded for MIPS32R5.

2587–2592 ↗(On Diff #139841)

These aliases should be guarded for MIPS32R5.

This revision now requires changes to proceed.Apr 4 2018, 4:25 AM

Comments addressed.

vstefanovic marked 9 inline comments as done.Apr 11 2018, 1:17 PM
sdardis added inline comments.Apr 12 2018, 4:02 AM
lib/Target/Mips/MicroMipsInstrInfo.td
593 ↗(On Diff #142066)

This requires:

let BaseOpcode = opstr;

like 'class LLEBaseMM'.

595 ↗(On Diff #142066)

As does this class.

1071–1084 ↗(On Diff #142066)

All of these require MMRel.

lib/Target/Mips/MipsInstrFormats.td
225 ↗(On Diff #142066)

This needs : StdArch

lib/Target/Mips/MipsInstrInfo.td
2504 ↗(On Diff #142066)

This requires:

let BaseOpcode = opstr;

2508–2515 ↗(On Diff #142066)

These all need MMRel. You'll also need to modify the MFC3OP and MTC3OP classes to have 'let BaseName = opstr'.

2528 ↗(On Diff #142066)

This shouldn't be part of the StdMMR6Rel instruction mapping table, the MMRel table is enough.

test/MC/Mips/virt/invalid.s
46–49 ↗(On Diff #142066)

You should reuse the same set of tests that you had for mthgc0 from lines 39-45. The check lines will differ obviously.

50–61 ↗(On Diff #142066)

For each unique instruction here, you should reuse the same set of tests that you had for mthgc0 from lines 38-45. The check lines will differ obviously.

Comments addressed.

vstefanovic marked 9 inline comments as done.Apr 12 2018, 12:50 PM

Only some minor nits now, inlined.

lib/Target/Mips/MicroMipsInstrInfo.td
1280–1293 ↗(On Diff #142236)

Deindent these, they should be aligned to the closing brace above them since they're not a part of the scope the the brace defines.

lib/Target/Mips/Mips64InstrInfo.td
562 ↗(On Diff #142236)

This should be MTC3OP<...>.

lib/Target/Mips/MipsInstrInfo.td
2715 ↗(On Diff #142236)

You can drop the braces here so the let clause only applies to the next definition/class.

test/MC/Mips/virt/valid64.s
6 ↗(On Diff #142236)

Once you fix the class definition of dmtgc0, this line will fail.

Fixed stupid error.

vstefanovic marked 4 inline comments as done.Apr 13 2018, 8:31 AM

LGTM, two minor nits that can be addressed on commit.

lib/Target/Mips/MipsInstrFormats.td
236–237 ↗(On Diff #142419)

Nit: align the '=' here vertically.

980 ↗(On Diff #142419)

Nit: align the '=' here vertically.

sdardis accepted this revision.Apr 26 2018, 8:33 AM
This revision is now accepted and ready to land.Apr 26 2018, 8:33 AM
This revision was automatically updated to reflect the committed changes.