[PATCH] D10611: [mips] Expansion of ROL and ROR macros

[Daniel Sanders]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM with a few minor changes (mostly redundant predicates)


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2733
@@ +2732,3 @@
+    
+  if (hasMips32r2() || hasMips32r6() || hasMips64r2() || hasMips64r6()) {
+    if (DReg == SReg) {
----------------
The last three predicates are redundant. Predicates are cumulative so hasMips32r2() covers all of them.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2734-2738
@@ +2733,7 @@
+  if (hasMips32r2() || hasMips32r6() || hasMips64r2() || hasMips64r6()) {
+    if (DReg == SReg) {
+      ATReg = getATReg(Inst.getLoc());
+      if (!ATReg)
+        return true;
+    }
+
----------------
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.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2778
@@ +2777,3 @@
+
+  if (hasMips32() || hasMips64()) {
+    switch (Inst.getOpcode()) {
----------------
hasMips64() is redundant.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2844
@@ +2843,3 @@
+
+  if (hasMips32r2() || hasMips32r6() || hasMips64r2() || hasMips64r6()) {
+
----------------
The last three are redundant.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2875
@@ +2874,3 @@
+
+  if (hasMips32() || hasMips64()) {
+
----------------
hasMips64() is redundant

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2947
@@ +2946,3 @@
+
+  if (hasMips64r2() || hasMips64r6()) {
+
----------------
hasMips64r6() is redundant

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2949-2953
@@ +2948,7 @@
+
+    if (DReg == SReg) {
+      ATReg = getATReg(Inst.getLoc());
+      if (!ATReg)
+        return true;
+    }
+
----------------
As noted in expandRotation(), assigning DReg to ATReg to a new variable (TmpReg?) would simplify lines 2959-2962 and 2972-2975.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3061
@@ +3060,3 @@
+
+  if (hasMips64r2() || hasMips64r6()) {
+
----------------
hasMips64r6() is redundant

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3084-3088
@@ +3083,7 @@
+    TmpInst.addOperand(MCOperand::createReg(SReg));
+    if (Inst.getOpcode() == Mips::DROLImm) {
+      TmpInst.addOperand(MCOperand::createImm((32 - ImmValue % 32) % 32));
+    } else {
+      TmpInst.addOperand(MCOperand::createImm(ImmValue % 32));
+    }
+    Instructions.push_back(TmpInst);
----------------
Nit: Redundant braces


http://reviews.llvm.org/D10611