[PATCH] D16323: [mips] Absolute value macro expansion
Daniel Sanders via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 22 06:30:47 PST 2016
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
More information about the llvm-commits
mailing list