This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS] Implement BC1EQZC, BC1NEZC, BC2EQZC and BC2NEZC instructions
ClosedPublic

Authored by zbuljan on Mar 22 2016, 7:41 AM.

Details

Summary

The patch implements microMIPSr6 BC1EQZC, BC1NEZC, BC2EQZC and BC2NEZC instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

zbuljan updated this revision to Diff 51286.Mar 22 2016, 7:41 AM
zbuljan retitled this revision from to [mips][microMIPS] Implement BC1EQZC, BC1NEZC, BC2EQZC and BC2NEZC instructions.
zbuljan updated this object.
zbuljan added subscribers: petarj, llvm-commits.
sdardis requested changes to this revision.Apr 19 2016, 8:08 AM
sdardis added a reviewer: sdardis.
sdardis added a subscriber: sdardis.

Small things mostly, comments inlined.

For the invalid/valid tests, can you sort the instructions you've added into alphabetical order?

lib/Target/Mips/MicroMips32r6InstrFormats.td
874 ↗(On Diff #51286)

Rename this to POOL32I_BRANCH_COP_1_2_FM_MMR6. The class below is a copy-paste which is then not necessary and so can be deleted.

881 ↗(On Diff #51286)

Just to be explicit, the encoding here is correct. The documentation isn't 100%.

887–898 ↗(On Diff #51286)

See my above comment.

lib/Target/Mips/MicroMips32r6InstrInfo.td
173–176 ↗(On Diff #51286)

See my comments on changing the class name to POOL32I_BRANCH_COP_1_2_FM_MMR6.

943 ↗(On Diff #51286)

Formatting, break the line after "BRANCH_DESC_BASE," and indent "HARDFLOAT {" appropriately.

test/MC/Disassembler/Mips/micromips32r6/valid.txt
261–268 ↗(On Diff #51286)

You don't need to check each instruction twice if there is no real difference between the operands.

Just once is sufficient provided you do not use zero values.

This applies to the rest of the */valid.s tests.

test/MC/Mips/micromips32r6/invalid.s
111–126 ↗(On Diff #51286)

For these invalid tests, checking once for each type of error for each instruction is sufficient.

This revision now requires changes to proceed.Apr 19 2016, 8:08 AM
sdardis added inline comments.Apr 20 2016, 2:09 AM
lib/Target/Mips/MicroMips32r6InstrInfo.td
943 ↗(On Diff #51286)

class BRANCH_COP1_MMR6_DESC also needs to inherit the relevant parameterised InstSE<> class.

This instruction should also define the register $at, like all other branches. $at is required to be defined by these instructions in case a long branch fixup needs to be emitted.

952 ↗(On Diff #51286)

This instruction needs to define at as well, but you can skip the InstSE<> class inheritance as COP2 is implementation specific and likely to be targeted by intrinsics anyway.

zbuljan updated this revision to Diff 56124.May 4 2016, 4:13 AM
zbuljan edited edge metadata.

Used only one format class POOL32I_BRANCH_COP_1_2_FM_MMR6 (other one was redundant so it is deleted).
Modified desc. class BRANCH_COP1_MMR6_DESC_BASE to inherit the relevant parameterised InstSE<> class.
Register $at is now defined in both desc. classes (BRANCH_COP1_MMR6_DESC_BASE and BRANCH_COP2_MMR6_DESC_BASE).
Removed redundant tests and added additional invalid tests.

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

One change and one nit. The inheritance order needs to be changed so that the various fields of the instruction records get set correctly. Comments inlined.

lib/Target/Mips/MicroMips32r6InstrFormats.td
982 ↗(On Diff #56124)

This class needs to inherit from MMR6Arch, as the current ordering does not set the Arch and BaseOpcode properly. The inheritance ordering should be similar to that of the existing microMIPSR6 branches, such as BGEC_MMR6.

lib/Target/Mips/MicroMips32r6InstrInfo.td
1140–1142 ↗(On Diff #56124)

As coprocessor 2 is implementation dependant, $ft should be $rt.

This revision now requires changes to proceed.May 6 2016, 3:45 AM
zbuljan updated this revision to Diff 57339.May 16 2016, 5:28 AM
zbuljan edited edge metadata.

Rebased to work with TOT.
Changed POOL32I_BRANCH_COP_1_2_FM_MMR6 format class to inherit from MMR6Arch.

zbuljan updated this revision to Diff 57564.May 18 2016, 12:46 AM
zbuljan edited edge metadata.

Rebased to work with TOT.

sdardis accepted this revision.May 18 2016, 4:13 AM
sdardis edited edge metadata.

LGTM with 2 tiny nits.

test/MC/Mips/micromips32r6/valid.s
338–345 ↗(On Diff #57564)

Checking each instruction once is sufficient. Just avoid using $zero or 0 as an operand because that may mask errors if values default to zero.

test/MC/Mips/micromips64r6/valid.s
276–283 ↗(On Diff #57564)

Here too.

This revision is now accepted and ready to land.May 18 2016, 4:13 AM
This revision was automatically updated to reflect the committed changes.