Details
Diff Detail
Event Timeline
There's quite a lot of formatting and coding standard nits in this patch. I'd recommend looking into clang-format-diff.py or git-clang-format since this will automate fixing most of the formatting nits that occur in the C++.
The main non-formatting comment I have is that this patch doesn't seem to account for the case when ror/rol/rorv/rolv are instructions rather than macros. It looks like it will expand even when the instructions are available. The other one is that the zero-immediate case should only emit a single instruction.
| lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
|---|---|---|
| 2467 | Is this 80 cols? It looks rather long in phabricator | |
| 2474–2475 | Nit: Avoid abbreviations unless they are well known. | |
| 2477–2490 | Nit: Indendation. Case labels should be at the same level as the switch. | |
| 2479 | Nit: Indentation of this particular line | |
| 2481 | This case is not reachable (it calls expandRotationImm() instead) | |
| 2486 | This case is not reachable (it calls expandRotationImm() instead) | |
| 2494–2496 | Nit: Don't use unnecessary braces. | |
| 2500 | Nit: Variables should start with a capital | |
| 2537–2538 | Nit: Indentation. Format it as per clang-format | |
| 2537–2597 | Most of the nits for expandRotation() also apply here. | |
| 2545–2546 | Nit: Avoid abbreviations unless they are well known | |
| lib/Target/Mips/MipsInstrInfo.td | ||
| 1557–1560 | Nit: Indentation. | |
| 1566–1569 | Nit: Indentation | |
| test/MC/Mips/rotations.s | ||
| 9 | Nit: Alignment of '# encoding' likewise below. | |
Daniel, my task was the expansion of these macros, by mips bugzilla report 723.
I believe that adding these as instructions on supported platforms is a whole different thing. For the start, we need information on which architectures these instructions exist and find their encodings. MIPS64 Instructions Set mentions only ROTR and ROTRV, and these two are already implemented.
I believe that this piece of code will not introduce any critical problems on platforms which support ROL/ROR instructions.
I am also preparing similar patch for DROL/DROR macros.
I believe it's closely related. On MIPS32r2 and later, ror/rol are aliases for rotr and rorv/rolv are aliases for rotrv/rotlv. Prior to MIPS32r2, they are macros that expand to equivalent code as per your patch.
Either way, you need to account for the case when the rotation immediate is zero.
New diff contains ROL/ROR expansion for all 32rN and 64rN platforms, where these instructions are aliasses for ROTR and ROTRV.
LGTM with a few minor changes (mostly redundant predicates)
| lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
|---|---|---|
| 2479 | The last three predicates are redundant. Predicates are cumulative so hasMips32r2() covers all of them. | |
| 2480–2484 | If this were: unsigned TmpReg = DReg;
if (DReg == SReg) {
TmpReg = getATReg(Inst.getLoc());
if (!TmpReg)
return true;
}then you wouldn't need to conditionally select between ATReg and DReg later on lines 2744-2747 and 2757-2760. You could use TmpReg instead. | |
| 2524 | hasMips64() is redundant. | |
| 2590 | The last three are redundant. | |
| 2621 | hasMips64() is redundant | |
| 2693 | hasMips64r6() is redundant | |
| 2695–2699 | As noted in expandRotation(), assigning DReg to ATReg to a new variable (TmpReg?) would simplify lines 2959-2962 and 2972-2975. | |
| 2807 | hasMips64r6() is redundant | |
| 2830–2834 | Nit: Redundant braces | |
New diff contains nit fixes
I'm not sure if phabricator is doing something strange here but I only see two of the nine nits fixed. The redundant braces and indentation nits are fixed but the rest don't seem to have been done.
New diff contains nit corrections, usage of TmpReg as suggested, and necessary changes to be applicable to latest source code.
New diff contains adaptations to the latest source code, using emit* functions for generating instructions.
Is this 80 cols? It looks rather long in phabricator