[PATCH] D16674: [mips] Expansion of SC[D] and LL[D] for R6
Simon Dardis via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 7 05:10:51 PDT 2016
sdardis requested changes to this revision.
sdardis added a comment.
This revision now requires changes to proceed.
This patch needs to be rebased due to structural changes in the files it touches. emitRRRI and emitRRRX have to be moved to MCTargetDesc/MipsTargetStreamer.cpp, expandLlSc has to acquire and use MipsTargetStreamer for the various emit functions, along with passing the SubtargetInfo object around. The SmallVectorImp<MCInst> has been dropped.
Comments inlined. The major issue I see is that using loadImmediate to synthesise the address into a register causes the address to be truncated to 32bits due to the use of addu. This is OK for N32 and O32, but will cause failures on N64 if the address lies outside the high 32bits.
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:227-229
@@ -226,2 +226,5 @@
+
+ bool expandLlSc(MCInst &Inst, SMLoc IDLoc,
+ SmallVectorImpl<MCInst> &Instructions, bool IsMips64);
bool expandRotation(MCInst &Inst, SMLoc IDLoc,
----------------
The name IsMips64 is deceptive. I think this parameter should be renamed Is64bitAtomic as it controls whether the instruction being expanded is the 64bit variant.
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1503-1521
@@ -1499,2 +1502,21 @@
+void emitRRRX(unsigned Opcode, unsigned Reg0, unsigned Reg1, unsigned Reg2,
+ MCOperand Op2, SMLoc IDLoc,
+ SmallVectorImpl<MCInst> &Instructions) {
+ MCInst tmpInst;
+ tmpInst.setOpcode(Opcode);
+ tmpInst.addOperand(MCOperand::createReg(Reg0));
+ tmpInst.addOperand(MCOperand::createReg(Reg1));
+ tmpInst.addOperand(MCOperand::createReg(Reg2));
+ tmpInst.addOperand(Op2);
+ tmpInst.setLoc(IDLoc);
+ Instructions.push_back(tmpInst);
+}
+
+void emitRRRI(unsigned Opcode, unsigned Reg0, unsigned Reg1, unsigned Reg2,
+ int16_t Imm, SMLoc IDLoc, SmallVectorImpl<MCInst> &Instructions) {
+ emitRRRX(Opcode, Reg0, Reg1, Reg2, MCOperand::createImm(Imm), IDLoc,
+ Instructions);
+}
+
void emitAppropriateDSLL(unsigned DstReg, unsigned SrcReg, int16_t ShiftAmount,
----------------
This now goes in lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp
These should be members of MipsTargetStreamer. tmpInst in emitRRRX should be TmpInst for consistency with the rest of the file.
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3213
@@ +3212,3 @@
+ SmallVectorImpl<MCInst> &Instructions,
+ bool IsMips64) {
+
----------------
The name IsMips64 is deceptive. It is perfectly valid to use ll+sc on mips64. I think this parameter should be renamed Is64bitAtomic.
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3238
@@ +3237,3 @@
+ // If 9 bit offset
+ if (OffsetValue > -257 && OffsetValue < 256) {
+ if (isLL) {
----------------
Daniel pointed this out to me, you can use isInt<9> rather than check the range manually.
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3249
@@ +3248,3 @@
+ // If 16 bit offset
+ else if (OffsetValue > -32769 && OffsetValue < 32768) {
+ emitRRI(Mips::ADDiu, TmpReg, SrcReg, OffsetValue, IDLoc, Instructions);
----------------
You can use isInt<16> rather than writing the range check by hand.
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3259-3271
@@ +3258,15 @@
+ } else {
+ bool Is32Bit = isInt<32>(OffsetValue) || isUInt<32>(OffsetValue);
+ loadImmediate(OffsetValue, TmpReg, Mips::NoRegister, Is32Bit, false, IDLoc,
+ Instructions);
+ emitRRR(Mips::ADDu, TmpReg, TmpReg, SrcReg, IDLoc, Instructions);
+ if (isLL) {
+ emitRRI(LLR6op, DstReg, TmpReg, 0, IDLoc, Instructions);
+ return false;
+ } else if (isSC) {
+ emitRRRI(SCR6op, DstReg, DstReg, TmpReg, 0, IDLoc, Instructions);
+ return false;
+ }
+ }
+ return true;
+}
----------------
This is producing semi-correct code that only works for N32 on mips64r6.
With this patch, out of range ll/sc get expanded to:
24: 27a20108 addiu v0,sp,264 <- wrong
28: 7c420037 lld v0,0(v0)
2c: 64430001 daddiu v1,v0,1
30: 27a10108 addiu at,sp,264 <- wrong
34: 7c230027 scd v1,0(at)
addiu cannot be used for 64 bit pointer manipulation as it's a 32 bit operation only. In contrast GAS generates:
24: 67a20108 daddiu v0,sp,264
28: 7c420037 lld v0,0(v0)
2c: 64430001 daddiu v1,v0,1
30: 67a10108 daddiu at,sp,264
34: 7c230027 scd v1,0(at)
The fault lies with loadImmediate as it's treating the immediate as a 32bit value on a 64 bit machine.
================
Comment at: lib/Target/Mips/Mips32r6InstrInfo.td:850
@@ +849,3 @@
+def SCMacro : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rt, mem:$addr),
+ "sc\t$rt, $addr">, ISA_MIPS32R6;
+
----------------
Align the assembly fragment to under the first '('.
================
Comment at: lib/Target/Mips/Mips32r6InstrInfo.td:853
@@ +852,2 @@
+def LLMacro : MipsAsmPseudoInst<(outs GPR32Opnd:$rt), (ins mem:$addr),
+ "ll\t$rt, $addr">, ISA_MIPS32R6;
----------------
Here too.
================
Comment at: lib/Target/Mips/Mips64r6InstrInfo.td:222
@@ +221,3 @@
+def SCDMacro : MipsAsmPseudoInst<(outs), (ins GPR64Opnd:$rt, mem:$addr),
+ "scd\t$rt, $addr">, ISA_MIPS64R6;
+
----------------
Align the assembly fragment to under the first '('.
================
Comment at: lib/Target/Mips/Mips64r6InstrInfo.td:225
@@ +224,2 @@
+def LLDMacro : MipsAsmPseudoInst<(outs GPR64Opnd:$rt), (ins mem:$addr),
+ "lld\t$rt, $addr">, ISA_MIPS64R6;
----------------
Here too.
================
Comment at: test/MC/Mips/ll_r6.s:1
@@ +1,2 @@
+# RUN: llvm-mc %s -triple=mipsel-unknown-linux -mcpu=mips32r6 -show-encoding | FileCheck %s
+
----------------
These test files should be called macro-<instruction>-r6.s
================
Comment at: test/MC/Mips/lld_r6.s:1-53
@@ +1,53 @@
+# RUN: llvm-mc %s -triple=mipsel-unknown-linux -mcpu=mips64r6 -show-encoding | FileCheck %s
+
+lld $2, 128($sp)
+# CHECK: lld $2, 128($sp) # encoding: [0x37,0x40,0xa2,0x7f]
+
+lld $2, -128($sp)
+# CHECK: lld $2, -128($sp) # encoding: [0x37,0xc0,0xa2,0x7f]
+
+lld $2, 255($sp)
+# CHECK: lld $2, 255($sp) # encoding: [0xb7,0x7f,0xa2,0x7f]
+
+lld $2, 256($sp)
+# CHECK: addiu $2, $sp, 256 # encoding: [0x00,0x01,0xa2,0x27]
+# CHECK: lld $2, 0($2) # encoding: [0x37,0x00,0x42,0x7c]
+
+lld $2, -256($sp)
+# CHECK: lld $2, -256($sp) # encoding: [0x37,0x80,0xa2,0x7f]
+
+lld $2, -257($sp)
+# CHECK: addiu $2, $sp, -257 # encoding: [0xff,0xfe,0xa2,0x27]
+# CHECK: lld $2, 0($2) # encoding: [0x37,0x00,0x42,0x7c]
+
+
+lld $2, 32767($sp)
+# CHECK: addiu $2, $sp, 32767 # encoding: [0xff,0x7f,0xa2,0x27]
+# CHECK: lld $2, 0($2) # encoding: [0x37,0x00,0x42,0x7c]
+
+lld $2, 32768($sp)
+# CHECK: ori $2, $zero, 32768 # encoding: [0x00,0x80,0x02,0x34]
+# CHECK: addu $2, $2, $sp # encoding: [0x21,0x10,0x5d,0x00]
+# CHECK: lld $2, 0($2) # encoding: [0x37,0x00,0x42,0x7c]
+
+lld $2, -32768($sp)
+# CHECK: addiu $2, $sp, -32768 # encoding: [0x00,0x80,0xa2,0x27]
+# CHECK: lld $2, 0($2) # encoding: [0x37,0x00,0x42,0x7c]
+
+lld $2, -32769($sp)
+# CHECK: lui $2, 65535 # encoding: [0xff,0xff,0x02,0x3c]
+# CHECK: ori $2, $2, 32767 # encoding: [0xff,0x7f,0x42,0x34]
+# CHECK: addu $2, $2, $sp # encoding: [0x21,0x10,0x5d,0x00]
+# CHECK: lld $2, 0($2) # encoding: [0x37,0x00,0x42,0x7c]
+
+lld $2, 655987($sp)
+# CHECK: lui $2, 10 # encoding: [0x0a,0x00,0x02,0x3c]
+# CHECK: ori $2, $2, 627 # encoding: [0x73,0x02,0x42,0x34]
+# CHECK: addu $2, $2, $sp # encoding: [0x21,0x10,0x5d,0x00]
+# CHECK: lld $2, 0($2) # encoding: [0x37,0x00,0x42,0x7c]
+
+lld $2, -655987($sp)
+# CHECK: lui $2, 65525 # encoding: [0xf5,0xff,0x02,0x3c]
+# CHECK: ori $2, $2, 64909 # encoding: [0x8d,0xfd,0x42,0x34]
+# CHECK: addu $2, $2, $sp # encoding: [0x21,0x10,0x5d,0x00]
+# CHECK: lld $2, 0($2) # encoding: [0x37,0x00,0x42,0x7c]
----------------
All these addus are incorrect, they should be daddus.
================
Comment at: test/MC/Mips/scd_r6.s:1-49
@@ +1,50 @@
+# RUN: llvm-mc %s -triple=mipsel-unknown-linux -mcpu=mips64r6 -show-encoding | FileCheck %s
+
+scd $2, 128($sp)
+# CHECK: scd $2, 128($sp) # encoding: [0x27,0x40,0xa2,0x7f]
+
+scd $2, -128($sp)
+# CHECK: scd $2, -128($sp) # encoding: [0x27,0xc0,0xa2,0x7f]
+
+scd $2, 255($sp)
+# CHECK: scd $2, 255($sp) # encoding: [0xa7,0x7f,0xa2,0x7f]
+
+scd $2, 256($sp)
+# CHECK: addiu $1, $sp, 256 # encoding: [0x00,0x01,0xa1,0x27]
+# CHECK: scd $2, 0($1) # encoding: [0x27,0x00,0x22,0x7c]
+
+scd $2, -256($sp)
+# CHECK: scd $2, -256($sp) # encoding: [0x27,0x80,0xa2,0x7f]
+
+scd $2, -257($sp)
+# CHECK: addiu $1, $sp, -257 # encoding: [0xff,0xfe,0xa1,0x27]
+# CHECK: scd $2, 0($1) # encoding: [0x27,0x00,0x22,0x7c]
+
+scd $2, 32767($sp)
+# CHECK: addiu $1, $sp, 32767 # encoding: [0xff,0x7f,0xa1,0x27]
+# CHECK: scd $2, 0($1) # encoding: [0x27,0x00,0x22,0x7c]
+
+scd $2, 32768($sp)
+# CHECK: ori $1, $zero, 32768 # encoding: [0x00,0x80,0x01,0x34]
+# CHECK: addu $1, $1, $sp # encoding: [0x21,0x08,0x3d,0x00]
+# CHECK: scd $2, 0($1) # encoding: [0x27,0x00,0x22,0x7c]
+
+scd $2, -32768($sp)
+# CHECK: addiu $1, $sp, -32768 # encoding: [0x00,0x80,0xa1,0x27]
+# CHECK: scd $2, 0($1) # encoding: [0x27,0x00,0x22,0x7c]
+
+scd $2, -32769($sp)
+# CHECK: lui $1, 65535 # encoding: [0xff,0xff,0x01,0x3c]
+# CHECK: ori $1, $1, 32767 # encoding: [0xff,0x7f,0x21,0x34]
+# CHECK: addu $1, $1, $sp # encoding: [0x21,0x08,0x3d,0x00]
+# CHECK: scd $2, 0($1) # encoding: [0x27,0x00,0x22,0x7c]
+
+scd $2, 655987($sp)
+# CHECK: lui $1, 10 # encoding: [0x0a,0x00,0x01,0x3c]
+# CHECK: ori $1, $1, 627 # encoding: [0x73,0x02,0x21,0x34]
+# CHECK: addu $1, $1, $sp # encoding: [0x21,0x08,0x3d,0x00]
+# CHECK: scd $2, 0($1) # encoding: [0x27,0x00,0x22,0x7c]
+
+scd $2, -655987($sp)
+# CHECK: lui $1, 65525 # encoding: [0xf5,0xff,0x01,0x3c]
+# CHECK: ori $1, $1, 64909 # encoding: [0x8d,0xfd,0x21,0x34]
----------------
Same here, daddu instead of addu.
http://reviews.llvm.org/D16674
More information about the llvm-commits
mailing list