The patch implements microMIPS64r6 DINSU, DINSM and DINS instructions.
Details
Diff Detail
Event Timeline
LGTM with some minor changes.
lib/Target/Mips/MicroMips64r6InstrInfo.td | ||
---|---|---|
98 | There's a couple things I ought to mention here:
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 | Naming nit: I'd like to keep the immediate size in the name if possible. Could we go with ConstantUImmRange5_Range2_64AsmOperandClass | |
542 | 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 | 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 | 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. | |
36 | 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 | Indentation on the '# encoding' | |
test/MC/Mips/mips64r2/invalid.s | ||
27–28 | dins should report 6-bit because there's a macro that expands to the appropriate dins* for each 6-bit immediate. |
lib/Target/Mips/MipsInstrInfo.td | ||
---|---|---|
428–429 | I agree with the name change. But you probably mean to name it "ConstantUImm5_Range2_64AsmOperandClass" (note the removal of "Range" part). | |
542 | Good to know, thanks for the heads up. | |
1236–1237 | 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 | You are right. Reverted back. | |
36 | Added. | |
test/MC/Mips/micromips64r6/valid.s | ||
153–155 | Fixed. |
There's a couple things I ought to mention here:
Neither of these issues require changes to this particular patch. I just mention them in case you encounter them.