[PATCH] D16323: [mips] Absolute value macro expansion

[Daniel Sanders via llvm-commits]

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

LGTM with a couple changes


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3537
@@ +3536,3 @@
+  emitRI(Mips::BGEZ, SecondRegOp, 8, IDLoc, Instructions);
+  emitRRR(Mips::ADDu, FirstRegOp, SecondRegOp, Mips::ZERO, IDLoc, Instructions);
+  emitRRR(Mips::SUB, FirstRegOp, Mips::ZERO, SecondRegOp, IDLoc, Instructions);
----------------
This should be a nop when both registers are the same. Assembling
  a: abs $2, $3
  b: abs $2, $2
  c:
gives:
  00000000 <a>:
     0:	04610002 	bgez	v1,c <b>
     4:	00601021 	move	v0,v1
     8:	00031022 	neg	v0,v1

  0000000c <b>:
     c:	04410002 	bgez	v0,18 <c>
    10:	00000000 	nop
    14:	00021022 	neg	v0,v0

  00000018 <c>:
  	...


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3539
@@ +3538,3 @@
+  emitRRR(Mips::SUB, FirstRegOp, Mips::ZERO, SecondRegOp, IDLoc, Instructions);
+  createNop(false, IDLoc, Instructions);
+
----------------
This nop should be deleted. Assuming you tested this the same way I did (by assembling then disassembling) then the nop you saw was padding to round off the section size.

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:1811-1812
@@ -1810,1 +1810,4 @@
 
+def ABS : MipsAsmPseudoInst<(outs GPR32Opnd:$rd), (ins GPR32Opnd:$rs),
+                            "abs\t$rd, $rs">;
+
----------------
Just a spelling nit: Could you make it obvious that it's a macro in the name? I'm thinking ABSMacro or ABSPseudo.

We ought to tidy up the existing names at some point but that's for another patch.

================
Comment at: test/MC/Mips/macro-abs.s:5-9
@@ +4,6 @@
+# CHECK:    .text
+  abs $4, $5
+# CHECK:    bgez    $5, 8       # encoding: [0x04,0xa1,0x00,0x02]
+# CHECK:    move    $4, $5      # encoding: [0x00,0xa0,0x20,0x21]
+# CHECK:    neg     $4, $5      # encoding: [0x00,0x05,0x20,0x22]
+# CHECK:    nop                 # encoding: [0x00,0x00,0x00,0x00]
----------------
Could you test 'abs $4, $4' too?


http://reviews.llvm.org/D16323