This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS][DSP] Implement PACKRL.PH, PICK.PH, PICK.QB, SHILO, SHILOV and WRDSP instructions
ClosedPublic

Authored by zbuljan on Nov 6 2015, 1:39 AM.

Details

Summary

The patch implements microMIPSDSP instructions: PACKRL.PH, PICK.PH, PICK.QB, SHILO, SHILOV and WRDSP.

Diff Detail

Repository
rL LLVM

Event Timeline

zbuljan updated this revision to Diff 39497.Nov 6 2015, 1:39 AM
zbuljan retitled this revision from to [mips][microMIPS][DSP] Implement PACKRL.PH, PICK.PH, PICK.QB, SHILO, SHILOV and WRDSP instructions.
zbuljan updated this object.
zbuljan added subscribers: petarj, llvm-commits.
dsanders requested changes to this revision.Nov 6 2015, 3:27 AM
dsanders edited edge metadata.

This is almost there but it introduces some dangerous 'let Predicates = ' statements. I'd like you to make an easy change to the existing DSP ASE to avoid it (and bring it into line with our recent work) as described below.

lib/Target/Mips/MipsDSPInstrInfo.td
1215 ↗(On Diff #39497)

I have to insist that we don't introduce more 'let Predicates = ' statements, they're dangerous since they can easily lose predicates.
Please port the DSP ASE to use the PredicateControl class (it's an easy change*) and use AdditionalPredicates for NotInMicroMips.

*Just add PredicateControl as a base class of DSPInst, set InsnPredicates appropriately in DSPInst, and adapt the existing 'let Predicates =' statements to adjectives or let statements on the sub-lists appropriately.

1450 ↗(On Diff #39497)

Likewise

This revision now requires changes to proceed.Nov 6 2015, 3:27 AM
zbuljan updated this revision to Diff 39682.Nov 9 2015, 5:55 AM
zbuljan edited edge metadata.

Existing DSP ASE changed so it use PredicateControl instead of 'let Predicates =' statements.
PredicateControl added to DSPInst and PseudoDSP as a base class.
'let Predicates =' replaced with 'let InsnPredicates ='.
Used AdditionalPredicates for NotInMicroMips.

zbuljan updated this revision to Diff 40631.Nov 19 2015, 4:48 AM
zbuljan edited edge metadata.

Rebased to work with top of the tree.
Old uimm10 operand definition replaced with new one which uses ConstantUImmAsmOperandClass.
Added uimm7_report_uimm and uimm10_report_uimm operands for implementation of WRDSP (MIPS version of WRDSP has 10-bit unsigned immediate while microMIPS version has 7-bit unsigned immediate).
Changed definition of simm6 operand to use new MipsSImmAsmOperandClass with DiagnosticType and SuperClasses members.
Implemented new match types in MipsAsmParser.cpp.
microMIPSDSPr1 assembler and disassembler tests added to microMIPSDSPr2 test files.
Updated expected error messages in invalid test files.

dsanders added inline comments.Dec 4 2015, 9:19 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
3358–3360 ↗(On Diff #40631)

Is there a reason to not give the range? I suspect this ought to be a plain Match_UImm7_0 similar to the others and the operand in the *_DESC should be updated accordingly.

I'm thinking it might be because it hits the same wrong-error issue that I encounter in my WIP patch for range-checked uimm16. For now I'm using the correct range-checked immediates and putting the problem cases in invalid-wrong-error.s files. I'm thinking that we should extend the error reporting to report multiple reasons for a failed match. For example:

filename.s:10:19: error: expected 7-bit unsigned immediate, or 10-bit unsigned immediate if '.set nomicromips' were used.
    wrdsp $2, -1
              ^~

This avoids the need to pick a single representative error message and is more helpful to the user.

3369 ↗(On Diff #40631)

Could you add 'signed' to the message so the user doesn't expect 0 - 63.

lib/Target/Mips/MipsDSPInstrInfo.td
20 ↗(On Diff #40631)

Is it possible for SHILO's immediate to be a relocatable expression? If not, we should is isConstantSImm

lib/Target/Mips/MipsInstrInfo.td
404–407 ↗(On Diff #40631)

You need to add ConstantUImm10AsmOperandClass to the list in the ConstantUImm4AsmOperandClass. This ensures instructions are matched in the correct order (uimm4 first). Otherwise, given the choice between uimm4 and uimm10 it might always pick uimm10 because that appears first in the matching table.

Similarly, MipsSImm6AsmOperandClass should be in this list too (and should be defined near here). I was planning to add the signed immediate just above the unsigned ones. I think MipsSImm6AsmOperandClass might need ConstantUImm10AsmOperandClass in its list too but I haven't tested this yet.

test/MC/Mips/micromips-dsp/invalid.s
24 ↗(On Diff #40631)

Missing column number. It will point at the mnemonic.

zbuljan updated this revision to Diff 42158.Dec 8 2015, 4:17 AM

Replaced Match_UImm7_0_Report_UImm with plain Match_UImm7_0.
Removed Match_UImm10_0_Report_UImm and used Match_UImm10_0 instead.
Error messages are updated accordingly.
SuperClass members of ConstantUImm7AsmOperandClass, ConstantSImm6AsmOperandClass and ConstantUImm5AsmOperandClass are updated so instructions can be matched in the correct order.
ConstantSImmAsmOperandClass and simm6 operand definition are moved to MipsInstrFormats.td (near the definition of ConstantUImmAsmOperandClass).
Added invalid-wrong-error.s for microMIPS DSP.
Problematic test cases for WRDSP instruction moved from invalid.s to invalid-wrong-error.s (they are correctly rejected but currently wrong error message is shown).

zbuljan updated this revision to Diff 42424.Dec 10 2015, 6:37 AM

Rebased to work with top of the tree.

dsanders accepted this revision.Dec 17 2015, 7:01 AM
dsanders edited edge metadata.

LGTM with one change I missed last time.

lib/Target/Mips/MipsInstrInfo.td
392 ↗(On Diff #42424)

Do you really need a ParserMethod? Integers should parse properly with the default parser.

This revision is now accepted and ready to land.Dec 17 2015, 7:01 AM
zbuljan added inline comments.Dec 18 2015, 12:54 AM
lib/Target/Mips/MipsInstrInfo.td
392 ↗(On Diff #42424)

I agree, ParserMethod is not needed here. I'll remove it from patch.

This revision was automatically updated to reflect the committed changes.