The patch implements microMIPS32r6 and microMIPS64r6 RECIP.fmt, RINT.fmt, ROUND.L.fmt, ROUND.W.fmt, SEL.fmt, SELEQZ.fmt, SELNEQZ.fmt and CLASS.fmt FPU instructions.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
If you're using the web interface to create the review, please remember to include the full context as described at http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
My main concern is the removal of several FPU instructions from 32R6 and 64R6.
| lib/Target/Mips/MicroMips32r6InstrInfo.td | ||
|---|---|---|
| 115 ↗ | (On Diff #37847) | RECIP -> RECIP_D |
| 117 ↗ | (On Diff #37847) | RINT -> RINT_D |
| 120 ↗ | (On Diff #37847) | ROUND_L -> ROUND_L_D |
| 124 ↗ | (On Diff #37847) | ROUND_W -> ROUND_W_D |
| 127 ↗ | (On Diff #37847) | SEL -> SEL_D |
| 714–725 ↗ | (On Diff #37847) | I should have mentioned this sooner but ABSS_FT_MMR6_DESC_BASE seems to have become a generic 2-operand description. This being the case, could you rename it (in a follow-up patch)? It seems strange to see RECIP.S (etc.) described as being an absolute operation. |
| 716 ↗ | (On Diff #37847) | RECIP -> RECIP_D |
| 720 ↗ | (On Diff #37847) | ROUND_L -> ROUND_L_D |
| 724 ↗ | (On Diff #37847) | ROUND_W -> ROUND_W_D |
| 728 ↗ | (On Diff #37847) | SEL -> SEL_D |
| 738 ↗ | (On Diff #37847) | RINT -> RINT_D |
| 1053 ↗ | (On Diff #37847) | RECIP -> RECIP_D |
| 1056 ↗ | (On Diff #37847) | RINT -> RINT_D |
| 1059 ↗ | (On Diff #37847) | ROUND_L -> ROUND_L_D |
| 1063 ↗ | (On Diff #37847) | ROUND_W -> ROUND_W_D |
| 1066 ↗ | (On Diff #37847) | SEL -> SEL_D |
| lib/Target/Mips/Mips32r6InstrInfo.td | ||
| 716–725 ↗ | (On Diff #37847) | Please move these back to where they were and use a separate 'let AdditionalPredicates ...'. They were in documentation order. |
| lib/Target/Mips/MipsInstrFPU.td | ||
| 270–279 ↗ | (On Diff #37847) | Why have you removed these from 32R6/64R6? 32R6/64R6 didn't change these instructions Also, multiple ISA_*'s on the same instruction doesn't work since it overrides the same sub-list from PredicateControl. |
LGTM with a possible nit
| lib/Target/Mips/MicroMips32r6InstrInfo.td | ||
|---|---|---|
| 1166 ↗ | (On Diff #40277) | 80 cols? It looks long but I haven't counted. |