This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS] Implement DINSU, DINSM, DINS instructions
ClosedPublic

Authored by hvarga on Jan 14 2016, 4:43 AM.

Details

Summary

The patch implements microMIPS64r6 DINSU, DINSM and DINS instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

hvarga updated this revision to Diff 44858.Jan 14 2016, 4:43 AM
hvarga retitled this revision from to [mips][microMIPS] Implement DINSU, DINSM, DINS instructions.
hvarga updated this object.
hvarga added subscribers: petarj, llvm-commits.
dsanders accepted this revision.Feb 10 2016, 8:51 AM
dsanders edited edge metadata.

LGTM with some minor changes.

lib/Target/Mips/MicroMips64r6InstrInfo.td
98 ↗(On Diff #44858)

There's a couple things I ought to mention here:

  • I recently discovered that the selection between dins/dinsm/dinsu is wrong, they don't use the same criteria as dext/dextm/dextu. It turned out IAS gets it wrong but GAS has been quietly fixing the bug for us (because it falls into the dins macro which expands to dinsm). One of my range checked immediate patches will correct the decision and make it
  • There is a constraint on the size operand of dinsm that we don't yet implement, in addition to 2<=Size<=64, there's also 32 < pos+size <= 64. I'm not aware of a way to check multiple operands in the assembly parser yet.

Neither of these issues require changes to this particular patch. I just mention them in case you encounter them.

lib/Target/Mips/MipsInstrInfo.td
428–429 ↗(On Diff #44858)

Naming nit: I'd like to keep the immediate size in the name if possible. Could we go with ConstantUImmRange5_Range2_64AsmOperandClass

542 ↗(On Diff #44858)

Just to mention it: My patch series deletes this function in favour of printUImm<BitSize>. Depending on what order we commit in you may need to change this.

1236–1237 ↗(On Diff #44858)

I noticed that we always use uimm5_inssize_plus1 as this argument. If that's the case, can we remove the argument and use it directly

test/MC/Mips/micromips64r6/invalid.s
21 ↗(On Diff #44858)

We haven't fixed this (and I'm not sure how we're going to). The valid range for size is determined by the value of pos.
Please restore the fixme.

36 ↗(On Diff #44858)

We've fixed this one, but could you add a fixme about the 32<=pos + size <= 64 constraint on dinsm

test/MC/Mips/micromips64r6/valid.s
153–155 ↗(On Diff #44858)

Indentation on the '# encoding'

test/MC/Mips/mips64r2/invalid.s
27–28 ↗(On Diff #44858)

dins should report 6-bit because there's a macro that expands to the appropriate dins* for each 6-bit immediate.

This revision is now accepted and ready to land.Feb 10 2016, 8:51 AM
hvarga marked 7 inline comments as done.Feb 25 2016, 4:52 AM
hvarga added inline comments.
lib/Target/Mips/MipsInstrInfo.td
428–429 ↗(On Diff #44858)

I agree with the name change. But you probably mean to name it "ConstantUImm5_Range2_64AsmOperandClass" (note the removal of "Range" part).

542 ↗(On Diff #44858)

Good to know, thanks for the heads up.

1236–1237 ↗(On Diff #44858)

Actually, there is a deviation in case of DINSM instruction. Class DINSM_MM64R6_DESC also inherits from InsBase class but uses uimm_range_2_64 as an argument instead of uimm5_inssize_plus1. So I will leave this class InsBase as is in this patch.

test/MC/Mips/micromips64r6/invalid.s
21 ↗(On Diff #44858)

You are right. Reverted back.

36 ↗(On Diff #44858)

Added.

test/MC/Mips/micromips64r6/valid.s
153–155 ↗(On Diff #44858)

Fixed.

This revision was automatically updated to reflect the committed changes.
hvarga marked 6 inline comments as done.