Page MenuHomePhabricator

[mips] MUL macro variations
ClosedPublic

Authored by sdardis on Feb 2 2016, 8:28 AM.

Details

Summary

[mips] MUL macro variations

Adds support for MUL macro variations.

Patch by: Srdjan Obucina

Diff Detail

Repository
rL LLVM

Event Timeline

obucina updated this revision to Diff 46657.Feb 2 2016, 8:28 AM
obucina retitled this revision from to [mips] MUL macro variations.
obucina updated this object.
obucina added reviewers: dsanders, zoran.jovanovic.
obucina added a subscriber: llvm-commits.

In original test example there is one more instruction

dmul $4, $5, $6

However, there is a DMUL instruction in TD files, available only for Cavium Octeon, with predicate [HasCnMips], so I am getting "error: instruction requires a CPU feature not currently enabled" when I try to process this instruction.

I need advice on how to handle this situation. One solution is to define new DMUL pseudo instruction for everything but CnMips. Existing DMUL appears as a processor specific instruction, and not a general one, so I am wondering about naming conventions, should I leave existing DMUL as is, or rename it to reflect its nature.

dsanders requested changes to this revision.Feb 3 2016, 6:48 AM
dsanders edited edge metadata.

In original test example there is one more instruction

dmul $4, $5, $6

However, there is a DMUL instruction in TD files, available only for Cavium Octeon, with predicate [HasCnMips], so I am getting "error: instruction requires a CPU feature not currently enabled" when I try to process this instruction.

I need advice on how to handle this situation. One solution is to define new DMUL pseudo instruction for everything but CnMips. Existing DMUL appears as a processor specific instruction, and not a general one, so I am wondering about naming conventions, should I leave existing DMUL as is, or rename it to reflect its nature.

Yes, adding a MipsAsmPseudoInst for the non-cnMIPS macro is the right way to go about it.

For the naming convention, instructions use the mnemonic as far as possible. A few instructions have conflicting definitions for the same mnemonic so these have a disambiguating suffix like '_R6', '_MM', or '_64'. For assembly macros, we've been moving towards using the mnemonic with a 'Macro' suffix (e.g. DMULMacro).

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
3587–3590 ↗(On Diff #46657)

Use the ternary operator

3607–3610 ↗(On Diff #46657)

Likewise

3612–3615 ↗(On Diff #46657)

Likewise

3617–3620 ↗(On Diff #46657)

What happens when useTraps() is true? Shouldn't this use a trap instruction instead of a break

3637–3640 ↗(On Diff #46657)

Use ternary operator

3643–3646 ↗(On Diff #46657)

What happens when useTraps() is true? Shouldn't this use a trap instruction instead of a break

lib/Target/Mips/MipsInstrInfo.td
1814–1831 ↗(On Diff #46657)

Naming convention as discussed above.

Also, AdditionalPredicates isn't the right predicate list for ISA predicates. Use the ISA_MIPS1_NOT_32R6_64R6 adjective instead.

1947 ↗(On Diff #46657)

AdditionalPredicates isn't the right predicate list for ISA predicates. Use the ISA_MIPS1_NOT_32R6_64R6 adjective instead.

This revision now requires changes to proceed.Feb 3 2016, 6:48 AM
obucina updated this revision to Diff 47324.Feb 9 2016, 8:37 AM
obucina edited edge metadata.
obucina marked 8 inline comments as done.

Changes according to the comments

obucina updated this revision to Diff 47326.Feb 9 2016, 8:46 AM
obucina edited edge metadata.

Old file accidentally left in the patch - removed.

DMUL for non CnMips still missing.

obucina updated this revision to Diff 47340.Feb 9 2016, 10:56 AM

Added DMUL macro for non CnMips. Please, take a look at the predicates of DMULMacro, and confirm are they OK.

dsanders requested changes to this revision.Mar 11 2016, 5:45 AM
dsanders edited edge metadata.

Added DMUL macro for non CnMips. Please, take a look at the predicates of DMULMacro, and confirm are they OK.

Those predicates look ok to me.

As future work, I think we might want to consider splitting InsnPredicates into a list of positive predicates and a list of negative predicates. Then we could use something like:
def ... : ..., ISA_MIPS64, ASE_NOT_CNMIPS.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
3593–3596 ↗(On Diff #47340)

Sorry, this isn't quite what I meant. I should have been clearer.

Rather than a ternary operator with assignments inside it, I was looking for something like:

emitRR(Inst.getOpcode() == Mips::MULImm ? Mips::MULT : Mips::DMULT, SReg, ATReg, IDLoc, Instructions);
3613–3616 ↗(On Diff #47340)

As above, this isn't quite what I meant.

3618–3621 ↗(On Diff #47340)

As above, this isn't quite what I meant.

3628 ↗(On Diff #47340)

I've just noticed that this isn't going to work for micromips because the offset is likely to be different for that case. If it's a constant offset then we can work around it for now (and leave a FIXME) but if it's variable then we'll need to find some way to do this.

For reference, my overall plan for solving this kind of problem is to finish rewriting the emit*() functions such that they directly write to the target streamer. This allows us to drop labels wherever we need them using the target streamer methods.

3643–3646 ↗(On Diff #47340)

As above, this isn't quite what I meant.

3659 ↗(On Diff #47340)

Similar to expandMulO(), this constant is different on micromips and might not be a constant.

3669 ↗(On Diff #47340)

Indentation

lib/Target/Mips/MipsInstrInfo.td
1816–1833 ↗(On Diff #47340)

MULImm and DMULImm should end in 'Macro' too

This revision now requires changes to proceed.Mar 11 2016, 5:45 AM
obucina updated this revision to Diff 51340.Mar 22 2016, 2:15 PM
obucina edited edge metadata.
obucina marked 5 inline comments as done.

According to feedback - using ternary operators.

obucina marked an inline comment as done.Mar 22 2016, 2:17 PM
obucina added inline comments.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
3691 ↗(On Diff #51340)

It is unclear to me what do you suggest regarding micromips potential problem with branch offset. Please be more precise.

3722 ↗(On Diff #51340)

It is unclear to me what do you suggest regarding micromips potential problem with branch offset. Please be more precise.

dsanders added inline comments.Mar 29 2016, 3:31 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
3691 ↗(On Diff #51340)

The offset needs to be different for microMIPS so you need to test for microMIPS and select between the MIPS and microMIPS value.
The way to fix this depends on whether the offset for the microMIPS case is a constant or is variable.

If the offset for the microMIPS case is a constant then we can use magic numbers for the moment (but please document them with comments). If the offset for the microMIPS case is variable for any reason then we'll have to find some way to account for the variability somehow (which could require the work explained at the bottom of this comment).

Ideally, we'd avoid any magic numbers using symbols and symbol references but it's not possible to do that right now (see below)

For reference, my overall plan for solving this kind of problem is to finish rewriting the emit*() functions such that they directly write to the
target streamer. This allows us to drop labels wherever we need them using the target streamer methods.

The reason we can't use symbols to avoid magic numbers at the moment is because this migration work is not yet complete. The problem is that we collect the expansion in a list of instructions and we can't put symbol definitions in this list. To eliminate the magic numbers we need to finish this work and use the target streamers as a stream. This will allow us to drop temporary symbols wherever we need them.

seanbruno requested changes to this revision.Sep 26 2016, 6:50 AM
seanbruno added a reviewer: seanbruno.
seanbruno added a subscriber: seanbruno.

This review seems to have aged out and needs the last comments addressed and a refresh to latest trunk.

This revision now requires changes to proceed.Sep 26 2016, 6:51 AM
sdardis commandeered this revision.Jan 30 2017, 7:24 AM
sdardis edited reviewers, added: obucina; removed: sdardis.
sdardis edited edge metadata.

Taking over this patch.

sdardis updated this revision to Diff 86285.Jan 30 2017, 7:28 AM
sdardis edited edge metadata.
sdardis marked 2 inline comments as done.
sdardis edited the summary of this revision. (Show Details)

Updated to trunk, addressed outstanding comment on directly specified branch operands vs. usage of label operands and labels.

sdardis updated this revision to Diff 86287.Jan 30 2017, 7:44 AM

Removed some unnecessary symbol manipulation.

seanbruno accepted this revision.Jan 30 2017, 2:59 PM

hrm ... I guess I can't "approve" this patch or something.

It requires @dsanders to accept the revision as he's requested changes previously.

dsanders accepted this revision.Feb 6 2017, 9:12 AM

It requires @dsanders to accept the revision as he's requested changes previously.

That must be a new requirement of phabricator. Marked as approved to let it change the overall state.

This revision is now accepted and ready to land.Feb 6 2017, 9:12 AM
This revision was automatically updated to reflect the committed changes.