[PATCH] Expansion of ROL and ROR instructions

[Daniel Sanders]

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.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2467
@@ -2451,1 +2466,3 @@
 
+bool MipsAsmParser::expandRotation(MCInst &Inst, SMLoc IDLoc, SmallVectorImpl<MCInst> &Instructions) {
+
----------------
Is this 80 cols? It looks rather long in phabricator

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2474-2475
@@ +2473,4 @@
+
+  unsigned FrstShift = Mips::NOP;
+  unsigned ScndShift = Mips::NOP;
+
----------------
Nit: Avoid abbreviations unless they are well known.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2477-2490
@@ +2476,16 @@
+
+  switch (Inst.getOpcode()) {
+      default:
+          llvm_unreachable("unexpected instruction opcode");
+      case Mips::ROL:
+      case Mips::ROLImm:
+        FrstShift = Mips::SRLV;
+        ScndShift = Mips::SLLV;
+        break;
+      case Mips::ROR:
+      case Mips::RORImm:
+        FrstShift = Mips::SLLV;
+        ScndShift = Mips::SRLV;
+        break;
+  }
+
----------------
Nit: Indendation. Case labels should be at the same level as the switch.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2479
@@ +2478,3 @@
+      default:
+          llvm_unreachable("unexpected instruction opcode");
+      case Mips::ROL:
----------------
Nit: Indentation of this particular line

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2481
@@ +2480,3 @@
+      case Mips::ROL:
+      case Mips::ROLImm:
+        FrstShift = Mips::SRLV;
----------------
This case is not reachable (it calls expandRotationImm() instead)

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2486
@@ +2485,3 @@
+      case Mips::ROR:
+      case Mips::RORImm:
+        FrstShift = Mips::SLLV;
----------------
This case is not reachable (it calls expandRotationImm() instead)

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2494-2496
@@ +2493,5 @@
+  SReg = Inst.getOperand(1).getReg();
+  if (Inst.getOperand(2).isReg()) {
+    TReg = Inst.getOperand(2).getReg();
+  }
+
----------------
Nit: Don't use unnecessary braces.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2500
@@ +2499,3 @@
+
+  MCInst tmpInst;
+
----------------
Nit: Variables should start with a capital

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2537-2538
@@ +2536,4 @@
+
+bool MipsAsmParser::expandRotationImm(MCInst &Inst, SMLoc IDLoc,
+                          SmallVectorImpl<MCInst> &Instructions) {
+
----------------
dsanders wrote:
> Most of the nits for expandRotation() also apply here.
Nit: Indentation. Format it as per clang-format

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2537-2597
@@ -2452,1 +2536,63 @@
+
+bool MipsAsmParser::expandRotationImm(MCInst &Inst, SMLoc IDLoc,
+                          SmallVectorImpl<MCInst> &Instructions) {
+
+  unsigned ATReg = Mips::NoRegister;
+  unsigned DReg = Mips::NoRegister;
+  unsigned SReg = Mips::NoRegister;
+  int64_t ImmValue = 0;
+
+  unsigned FrstShift = Mips::NOP;
+  unsigned ScndShift = Mips::NOP;
+
+  switch (Inst.getOpcode()) {
+      default:
+          llvm_unreachable("unexpected instruction opcode");
+      case Mips::ROLImm:
+        FrstShift = Mips::SLL;
+        ScndShift = Mips::SRL;
+        break;
+      case Mips::RORImm:
+        FrstShift = Mips::SRL;
+        ScndShift = Mips::SLL;
+        break;
+  }
+
+  DReg = Inst.getOperand(0).getReg();
+  SReg = Inst.getOperand(1).getReg();
+  if (Inst.getOperand(2).isImm()) {
+    ImmValue = Inst.getOperand(2).getImm();
+  }
+
+  ATReg = getATReg(Inst.getLoc());
+
+  MCInst tmpInst;
+
+  tmpInst.clear();
+  tmpInst.setLoc(Inst.getLoc());
+  tmpInst.setOpcode(FrstShift);
+  tmpInst.addOperand(MCOperand::createReg(ATReg));
+  tmpInst.addOperand(MCOperand::createReg(SReg));
+  tmpInst.addOperand(MCOperand::createImm(ImmValue));
+  Instructions.push_back(tmpInst);
+
+  tmpInst.clear();
+  tmpInst.setLoc(Inst.getLoc());
+  tmpInst.setOpcode(ScndShift);
+  tmpInst.addOperand(MCOperand::createReg(DReg));
+  tmpInst.addOperand(MCOperand::createReg(SReg));
+  tmpInst.addOperand(MCOperand::createImm(32 - ImmValue));
+  Instructions.push_back(tmpInst);
+
+  tmpInst.clear();
+  tmpInst.setLoc(Inst.getLoc());
+  tmpInst.setOpcode(Mips::OR);
+  tmpInst.addOperand(MCOperand::createReg(DReg));
+  tmpInst.addOperand(MCOperand::createReg(DReg));
+  tmpInst.addOperand(MCOperand::createReg(ATReg));
+  Instructions.push_back(tmpInst);
+
+  return false;
+}
+
 void MipsAsmParser::createNop(bool hasShortDelaySlot, SMLoc IDLoc,
----------------
Most of the nits for expandRotation() also apply here.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2545-2546
@@ +2544,4 @@
+
+  unsigned FrstShift = Mips::NOP;
+  unsigned ScndShift = Mips::NOP;
+
----------------
Nit: Avoid abbreviations unless they are well known

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:1557-1560
@@ -1556,1 +1556,6 @@
 
+def ROL : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rs, GPR32Opnd:$rt, GPR32Opnd:$rd),
+                      "rol\t$rs, $rt, $rd"> ;
+def ROLImm : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rs, GPR32Opnd:$rt, simm16:$imm),
+                      "rol\t$rs, $rt, $imm"> ;
+def : MipsInstAlias<"rol $rd, $rs",
----------------
Nit: Indentation.

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:1566-1569
@@ +1565,6 @@
+
+def ROR : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rs, GPR32Opnd:$rt, GPR32Opnd:$rd),
+                      "ror\t$rs, $rt, $rd"> ;
+def RORImm : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rs, GPR32Opnd:$rt, simm16:$imm),
+                      "ror\t$rs, $rt, $imm"> ;
+def : MipsInstAlias<"ror $rd, $rs",
----------------
Nit: Indentation

================
Comment at: test/MC/Mips/rotations.s:9
@@ +8,3 @@
+# CHECK: sllv	$4, $4, $5              # encoding: [0x00,0xa4,0x20,0x04]
+# CHECK: or	$4, $4, $1              # encoding: [0x00,0x81,0x20,0x25]
+  rol	$4,$5,$6
----------------
Nit: Alignment of '# encoding'

likewise below.

http://reviews.llvm.org/D10611

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/