This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS][DSP] Implement BALIGN, BITREV, BPOSGE32, CMP*, CMPGDU*, CMPGU* and CMPU* instructions
ClosedPublic

Authored by zbuljan on Jan 14 2016, 4:48 AM.

Details

Summary

The patch implements microMIPSDSP instructions: BALIGN, BITREV, BPOSGE32, CMP*, CMPGDU*, CMPGU* and CMPU*.

Diff Detail

Repository
rL LLVM

Event Timeline

zbuljan updated this revision to Diff 44861.Jan 14 2016, 4:48 AM
zbuljan retitled this revision from to [mips][microMIPS][DSP] Implement BALIGN, BITREV, BPOSGE32, CMP*, CMPGDU*, CMPGU* and CMPU* instructions.
zbuljan updated this object.
zbuljan added subscribers: petarj, llvm-commits.
zbuljan updated this revision to Diff 50393.Mar 11 2016, 12:17 AM
zbuljan edited edge metadata.

Rebased to work with top of the tree.
Added new template argument (DAGOperand) to existing BPOSGE32_DESC_BASE class.
Desc. class for microMIPSDSP BPOSGE32 now uses brtarget_mm operand instead of brtarget operand.
Removed unnecessary changes in encoder and decoder method of brtarget operand.

sdardis requested changes to this revision.Apr 25 2016, 3:14 AM
sdardis added a reviewer: sdardis.
sdardis added a subscriber: sdardis.
sdardis added inline comments.
lib/Target/Mips/MicroMipsDSPInstrFormats.td
279 ↗(On Diff #50393)

Pool occurs twice in the name of this class, is the second mention necessary?

lib/Target/Mips/MicroMipsDSPInstrInfo.td
392–404 ↗(On Diff #50393)

Can the DESC_BASE be turned into a parameterless class? BALIGN_MMR2_DESC seems to be the only class using the description BASE.

lib/Target/Mips/MipsDSPInstrInfo.td
1230–1232 ↗(On Diff #50393)

Why was ISA_DSPR2 removed? These instructions are only defined for DSP revision 2.

1270 ↗(On Diff #50393)

Same here, this instruction is defined for DSP revision 2.

This revision now requires changes to proceed.Apr 25 2016, 3:14 AM
mpf added a subscriber: mpf.Apr 25 2016, 3:55 AM

This patch mixes some independent features. If I remember correctly BALIGN and BITREV have been added to the MIPSR6 ISA (and microMIPSR6) and were previously only in DSPR2. The BPOSGE32C instruction is part of DSPR3 and is the only new instruction in DSPR3 with BPOSGE32 being removed from microMIPS DSPR3 but retained in MIPS DSPR3. The CMP* are pre-existing DSPR2 (or may even be DSPR1 I do not know from memory).

mpf added a comment.Apr 25 2016, 3:59 AM

Missed a few extra details... DSPR3 requires a minimum of MIPSR6/microMIPSR6 and DSPR1/2 is allowed with MIPSR6 but not allowed with microMIPSR6. The DSP ASE also has flags to set in the .MIPS.abiflags section if this is not already implemented. These details are partly important for correctness and partly for accurate tracking of object content. These need not be dealt with in this patch, just extra things to handle.

Thanks Matthew, extra change highlighted.

lib/Target/Mips/MicroMipsDSPInstrInfo.td
518 ↗(On Diff #50393)

This needs to be marked as being unavailable on microMIPSDSPR6.

zbuljan updated this revision to Diff 56400.May 6 2016, 4:50 AM
zbuljan edited edge metadata.

Rebased to work with TOT.
Class POOL32_3RB0_FMT renamed to POOL32_3RB0POOL_FMT.
Changed BALIGN_MMR2_DESC to be parameterless class.
Instruction BPOSGE32 separated from microMIPSDSPR6 using ISA_MIPS1_NOT_32R6_64R6 flag.
ISA_DSPR2 is returned to CMPGDU* and BALIGN because these instructions are for DSP2.
Added valid-dsp.txt and valid-dsp.s for test of microMIPSDSP BPOSGE32 instruction which is removed in microMIPSr6.

Test that bposge32 is invalid for microMIPS32r6 and microMIPS64r6 invalid.s tests along with a comment noting that it is a DSP instruction.

lib/Target/Mips/MicroMipsDSPInstrFormats.td
279 ↗(On Diff #56400)

"pool" does not need to be a parameter for this class. Bits 31-26 should correspond to POOL32A's opcode. And it should be named "POOL32A_3RB0_FMT". Sorry for the bikeshedding there.

303 ↗(On Diff #56400)

Add a class describing POOL32S 3 register bit0 format for cmpgu.<cond>.qb.

lib/Target/Mips/MicroMipsDSPInstrInfo.td
167–172 ↗(On Diff #56400)

These should inherit from POOL32S_3RB0_FMT.

test/MC/Disassembler/Mips/micromips32r3/valid-dsp.txt
3 ↗(On Diff #56400)

This should go in the micromips-dsp and micromips-dspr2 sets of tests.

test/MC/Mips/micromips/valid-dsp.s
4 ↗(On Diff #56400)

This should go in the relevant existing micromips-dsp/micromips-dspr2 tests.

sdardis requested changes to this revision.May 6 2016, 6:09 AM
sdardis edited edge metadata.

I hit comment by accident.

Thanks.
Simon

This revision now requires changes to proceed.May 6 2016, 6:09 AM
zbuljan updated this revision to Diff 57157.May 13 2016, 3:59 AM
zbuljan edited edge metadata.

Rebased to work with TOT.
Renamed format class POOL32_3RB0_FMT to POOL32S_3RB0_FMT and removed unnecessary parametar.
Test files for microMIPS32r3 + DSPr1 are placed in micromips-dsp folders.
Added invalid tests for BPOSGE32 (BPOSGE32 is microMIPS DSP instruction but it is removed in release 6).

sdardis accepted this revision.May 16 2016, 6:25 AM
sdardis edited edge metadata.

LGTM with cmp.cond.ph descriptions fixed along with the corresponding tests.

lib/Target/Mips/MicroMipsDSPInstrInfo.td
161–163 ↗(On Diff #57157)

If I'm reading this right, I suspect you have written down the instructions from the assembly format and written the minor opcodes assuming the encoding description table is in the same order. They are in a different order in the encoding description.

The minor opcodes for cmp.eq.ph and cmp.le.ph are swapped.

This revision is now accepted and ready to land.May 16 2016, 6:25 AM
This revision was automatically updated to reflect the committed changes.