[mips] MUL macro variations
Adds support for MUL macro variations.
Patch by: Srdjan Obucina
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
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.
Comment Actions
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 | ||
|---|---|---|
| 3656–3659 | Use the ternary operator | |
| 3676–3679 | Likewise | |
| 3681–3684 | Likewise | |
| 3686–3689 | What happens when useTraps() is true? Shouldn't this use a trap instruction instead of a break | |
| 3706–3709 | Use ternary operator | |
| 3712–3715 | What happens when useTraps() is true? Shouldn't this use a trap instruction instead of a break | |
| lib/Target/Mips/MipsInstrInfo.td | ||
| 2013–2030 | 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. | |
| 2147 | AdditionalPredicates isn't the right predicate list for ISA predicates. Use the ISA_MIPS1_NOT_32R6_64R6 adjective instead. | |
Comment Actions
Old file accidentally left in the patch - removed.
DMUL for non CnMips still missing.
Comment Actions
Added DMUL macro for non CnMips. Please, take a look at the predicates of DMULMacro, and confirm are they OK.
Comment Actions
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 | ||
|---|---|---|
| 3656–3659 | 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); | |
| 3676–3679 | As above, this isn't quite what I meant. | |
| 3681–3684 | As above, this isn't quite what I meant. | |
| 3691 | 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. | |
| 3706–3709 | As above, this isn't quite what I meant. | |
| 3722 | Similar to expandMulO(), this constant is different on micromips and might not be a constant. | |
| 3732 | Indentation | |
| lib/Target/Mips/MipsInstrInfo.td | ||
| 2013–2030 | MULImm and DMULImm should end in 'Macro' too | |
| lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
|---|---|---|
| 3691 | The offset needs to be different for microMIPS so you need to test for microMIPS and select between the MIPS and microMIPS value. 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)
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. | |
Comment Actions
This review seems to have aged out and needs the last comments addressed and a refresh to latest trunk.
Comment Actions
Updated to trunk, addressed outstanding comment on directly specified branch operands vs. usage of label operands and labels.
Comment Actions
That must be a new requirement of phabricator. Marked as approved to let it change the overall state.
Use the ternary operator