This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS] Implement RECIP.fmt, RINT.fmt, ROUND.L.fmt, ROUND.W.fmt, SEL.fmt, SELEQZ.fmt, SELNEQZ.fmt and CLASS.fmt
ClosedPublic

Authored by hvarga on Oct 20 2015, 3:50 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

hvarga updated this revision to Diff 37847.Oct 20 2015, 3:50 AM
hvarga retitled this revision from to [mips][microMIPS] Implement RECIP.fmt, RINT.fmt, ROUND.L.fmt, ROUND.W.fmt, SEL.fmt, SELEQZ.fmt, SELNEQZ.fmt and CLASS.fmt.
hvarga updated this object.
hvarga added subscribers: petarj, llvm-commits.
dsanders requested changes to this revision.Oct 22 2015, 7:33 AM
dsanders edited edge metadata.

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.

This revision now requires changes to proceed.Oct 22 2015, 7:33 AM
hvarga updated this revision to Diff 40277.Nov 16 2015, 5:13 AM
hvarga edited edge metadata.

Fix comments recieved from dsanders.

dsanders accepted this revision.Nov 27 2015, 1:54 AM
dsanders edited edge metadata.

LGTM with a possible nit

lib/Target/Mips/MicroMips32r6InstrInfo.td
1166 ↗(On Diff #40277)

80 cols? It looks long but I haven't counted.

This revision is now accepted and ready to land.Nov 27 2015, 1:54 AM
This revision was automatically updated to reflect the committed changes.