[Mips] Instruction `sc` now accepts symbol as an argument

[mbrkusanin]

Summary

Function MipsAsmParser::expandMemInst() did not properly handle instruction sc
with a symbol as an argument because first argument would be counted twice.
We add additional checks and handle this case separately.

[atanasyan]

I cannot invent any negative test case / example, but I'm concerned a bit that we completely drop information about out-register. The sc instruction has output result but we do not track that fact in td file just in our mind.

[mbrkusanin]

Alternative solution that does not remove out-register would look something like this: https://reviews.llvm.org/differential/diff/211116/

[mbrkusanin]

• Decided to apply diff of the alternative solution because the patch could not be downloaded with just the provided diff.

[atanasyan]

test/MC/Mips/sym-sc.s

52

Could you modify this test case or add a new one to check handling symbol and offset? Like this sc $12, some_random_symbol_name + 8.

[mbrkusanin]

Symbol with offset was not handled properly on MipsR6. R_MIPS_LO16 (MipsMCExpr::MEK_LO) would write offset into lower 16 bits, where as for R6 version offset is in bits 7-15. Initially I made new type of MCFixup which would only use those bits, but then there was a lot of additional code needed for handling all cases where MCFixups were used. Now we just add ADDIU for R6 and use R_MIPS_LO16 on that instruction which is the same way that gcc works.

[mbrkusanin]

• Removed unused function declarations.

[atanasyan]

LGTM with a few nits. I will fix them by myself before committing the patch.

lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp

348 ↗

(On Diff #213829)

clang-format

365 ↗

(On Diff #213829)

ditto

lib/Target/Mips/Disassembler/MipsDisassembler.cpp

static DecodeStatus DecodeMem(MCInst &Inst,
                               const void *Decoder) {
   int Offset = SignExtend32<16>(Insn & 0xffff);
   unsigned Reg = fieldFromInstruction(Insn, 16, 5);
   unsigned Base = fieldFromInstruction(Insn, 21, 5);
 
   Reg = getReg(Decoder, Mips::GPR32RegClassID, Reg);
   Base = getReg(Decoder, Mips::GPR32RegClassID, Base);
 
-  if (Inst.getOpcode() == Mips::SC ||
-      Inst.getOpcode() == Mips::SCD)
-    Inst.addOperand(MCOperand::createReg(Reg));
-
   Inst.addOperand(MCOperand::createReg(Reg));
   Inst.addOperand(MCOperand::createReg(Base));
   Inst.addOperand(MCOperand::createImm(Offset));
 
   return MCDisassembler::Success;
 }
 
 static DecodeStatus DecodeMemEVA(MCInst &Inst,
                                  unsigned Insn,
                                  uint64_t Address,
                                  const void *Decoder) {
   int Offset = SignExtend32<9>(Insn >> 7);
   unsigned Reg = fieldFromInstruction(Insn, 16, 5);
   unsigned Base = fieldFromInstruction(Insn, 21, 5);
 
   Reg = getReg(Decoder, Mips::GPR32RegClassID, Reg);
   Base = getReg(Decoder, Mips::GPR32RegClassID, Base);
 
-   if (Inst.getOpcode() == Mips::SCE)
-     Inst.addOperand(MCOperand::createReg(Reg));
-
   Inst.addOperand(MCOperand::createReg(Reg));
   Inst.addOperand(MCOperand::createReg(Base));
   Inst.addOperand(MCOperand::createImm(Offset));
 
   return MCDisassembler::Success;
 }
 
 static DecodeStatus DecodeLoadByte15(MCInst &Inst,
static DecodeStatus DecodeMemMMImm9(MCInst &Inst,
                                     const void *Decoder) {
   int Offset = SignExtend32<9>(Insn & 0x1ff);
   unsigned Reg = fieldFromInstruction(Insn, 21, 5);
   unsigned Base = fieldFromInstruction(Insn, 16, 5);
 
   Reg = getReg(Decoder, Mips::GPR32RegClassID, Reg);
   Base = getReg(Decoder, Mips::GPR32RegClassID, Base);
 
-  if (Inst.getOpcode() == Mips::SCE_MM || Inst.getOpcode() == Mips::SC_MMR6)
-    Inst.addOperand(MCOperand::createReg(Reg));
-
   Inst.addOperand(MCOperand::createReg(Reg));
   Inst.addOperand(MCOperand::createReg(Base));
   Inst.addOperand(MCOperand::createImm(Offset));
 
   return MCDisassembler::Success;
 }
 
 static DecodeStatus DecodeMemMMImm12(MCInst &Inst,
static DecodeStatus DecodeMemMMImm12(MCInst &Inst,
   case Mips::SWM32_MM:
   case Mips::LWM32_MM:
     if (DecodeRegListOperand(Inst, Insn, Address, Decoder)
         == MCDisassembler::Fail)
       return MCDisassembler::Fail;
     Inst.addOperand(MCOperand::createReg(Base));
     Inst.addOperand(MCOperand::createImm(Offset));
     break;
-  case Mips::SC_MM:
-    Inst.addOperand(MCOperand::createReg(Reg));
-    LLVM_FALLTHROUGH;
   default:
     Inst.addOperand(MCOperand::createReg(Reg));
     if (Inst.getOpcode() == Mips::LWP_MM || Inst.getOpcode() == Mips::SWP_MM)
       Inst.addOperand(MCOperand::createReg(Reg+1));
 
     Inst.addOperand(MCOperand::createReg(Base));
     Inst.addOperand(MCOperand::createImm(Offset));
   }
static DecodeStatus DecodeSpecial3LlSc(MCInst &Inst,
                                        const void *Decoder) {
   int64_t Offset = SignExtend64<9>((Insn >> 7) & 0x1ff);
   unsigned Rt = fieldFromInstruction(Insn, 16, 5);
   unsigned Base = fieldFromInstruction(Insn, 21, 5);
 
   Rt = getReg(Decoder, Mips::GPR32RegClassID, Rt);
   Base = getReg(Decoder, Mips::GPR32RegClassID, Base);
 
-  if(Inst.getOpcode() == Mips::SC_R6 || Inst.getOpcode() == Mips::SCD_R6){
-    Inst.addOperand(MCOperand::createReg(Rt));
-  }
-
   Inst.addOperand(MCOperand::createReg(Rt));
   Inst.addOperand(MCOperand::createReg(Base));
   Inst.addOperand(MCOperand::createImm(Offset));
 
   return MCDisassembler::Success;
 }
 
 static DecodeStatus DecodeHWRegsRegisterClass(MCInst &Inst,

lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp

bool isBasePlusOffsetMemoryAccess(unsigned Opcode, unsigned *AddrIdx,
   // Store instructions with base address register in position 1.
   case Mips::SB:
   case Mips::SH:
   case Mips::SW:
   case Mips::SWC1:
   case Mips::SDC1:
   case Mips::SWL:
   case Mips::SWR:
-    *AddrIdx = 1;
-    if (IsStore)
-      *IsStore = true;
-    return true;
-
-  // Store instructions with base address register in position 2.
   case Mips::SC:
   case Mips::SC_R6:
-    *AddrIdx = 2;
+    *AddrIdx = 1;
     if (IsStore)
       *IsStore = true;
     return true;
   }
 }
 
 bool baseRegNeedsLoadStoreMask(unsigned Reg) {
   // The contents of SP and thread pointer register do not require masking.

lib/Target/Mips/MicroMips32r6InstrInfo.td

class GINVI_MMR6_DESC : GINV_MMR6_DESC_BASE<"ginvi", GPR32Opnd,
                                             II_GINVI> {
   dag InOperandList = (ins GPR32Opnd:$rs);
   string AsmString = "ginvi\t$rs";
 }
 class GINVT_MMR6_DESC : GINV_MMR6_DESC_BASE<"ginvt", GPR32Opnd,
                                             II_GINVT>;
 
 class SC_MMR6_DESC_BASE<string opstr, InstrItinClass itin> {
-  dag OutOperandList = (outs GPR32Opnd:$dst);
   dag InOperandList = (ins GPR32Opnd:$rt, mem_mm_9:$addr);
   string AsmString = !strconcat(opstr, "\t$rt, $addr");
   InstrItinClass Itinerary = itin;
   string BaseOpcode = opstr;
   bit mayStore = 1;
-  string Constraints = "$rt = $dst";
   string DecoderMethod = "DecodeMemMMImm9";
 }
 
 class LL_MMR6_DESC_BASE<string opstr, InstrItinClass itin> {
   dag OutOperandList = (outs GPR32Opnd:$rt);
   dag InOperandList = (ins mem_mm_9:$addr);
   string AsmString = !strconcat(opstr, "\t$rt, $addr");
   InstrItinClass Itinerary = itin;

lib/Target/Mips/MicroMipsInstrInfo.td

class LLEBaseMM<string opstr, RegisterOperand RO> :
   InstSE<(outs RO:$rt), (ins mem_simm9:$addr),
          !strconcat(opstr, "\t$rt, $addr"), [], II_LLE, FrmI> {
   let DecoderMethod = "DecodeMemMMImm9";
   string BaseOpcode = opstr;
   let mayLoad = 1;
 }
 
 class SCBaseMM<string opstr, RegisterOperand RO> :
-  InstSE<(outs RO:$dst), (ins RO:$rt, mem_mm_12:$addr),
+  InstSE<(outs), (ins RO:$rt, mem_mm_12:$addr),
          !strconcat(opstr, "\t$rt, $addr"), [], II_SC, FrmI> {
   let DecoderMethod = "DecodeMemMMImm12";
   let mayStore = 1;
-  let Constraints = "$rt = $dst";
 }
 
 class SCEBaseMM<string opstr, RegisterOperand RO> :
-  InstSE<(outs RO:$dst), (ins RO:$rt, mem_simm9:$addr),
+  InstSE<(outs), (ins RO:$rt, mem_simm9:$addr),
          !strconcat(opstr, "\t$rt, $addr"), [], II_SCE, FrmI> {
   let DecoderMethod = "DecodeMemMMImm9";
   string BaseOpcode = opstr;
   let mayStore = 1;
-  let Constraints = "$rt = $dst";
 }
 
 class LoadMM<string opstr, DAGOperand RO, SDPatternOperator OpNode = null_frag,
              InstrItinClass Itin = NoItinerary, DAGOperand MO = mem_mm_12> :
   InstSE<(outs RO:$rt), (ins MO:$addr),
          !strconcat(opstr, "\t$rt, $addr"),
          [(set RO:$rt, (OpNode addrimm12:$addr))], Itin, FrmI, opstr> {
   let DecoderMethod = "DecodeMemMMImm12";

lib/Target/Mips/Mips32r6InstrInfo.td

class LL_R6_DESC_BASE<string instr_asm, RegisterOperand GPROpnd,
   bit mayLoad = 1;
   InstrItinClass Itinerary = itin;
 }
 
 class LL_R6_DESC : LL_R6_DESC_BASE<"ll", GPR32Opnd, mem_simm9, II_LL>;
 
 class SC_R6_DESC_BASE<string instr_asm, RegisterOperand GPROpnd,
                       InstrItinClass itin> {
-  dag OutOperandList = (outs GPROpnd:$dst);
   dag InOperandList = (ins GPROpnd:$rt, mem_simm9:$addr);
   string AsmString = !strconcat(instr_asm, "\t$rt, $addr");
   list<dag> Pattern = [];
   bit mayStore = 1;
-  string Constraints = "$rt = $dst";
   InstrItinClass Itinerary = itin;
 }
 
 class SC_R6_DESC : SC_R6_DESC_BASE<"sc", GPR32Opnd, II_SC>;
 
 class CLO_CLZ_R6_DESC_BASE<string instr_asm, RegisterOperand GPROpnd,
                            InstrItinClass itin>
     : MipsR6Arch<instr_asm> {

lib/Target/Mips/MipsEVAInstrInfo.td

class LLE_DESC_BASE<string instr_asm, RegisterOperand GPROpnd,
   string DecoderMethod = "DecodeMemEVA";
   InstrItinClass Itinerary = itin;
 }
 
 class LLE_DESC : LLE_DESC_BASE<"lle", GPR32Opnd, II_LLE>;
 
 class SCE_DESC_BASE<string instr_asm, RegisterOperand GPROpnd,
                     InstrItinClass itin = NoItinerary> {
-  dag OutOperandList = (outs GPROpnd:$dst);
   dag InOperandList = (ins GPROpnd:$rt, mem_simm9:$addr);
   string AsmString = !strconcat(instr_asm, "\t$rt, $addr");
   list<dag> Pattern = [];
   string BaseOpcode = instr_asm;
   bit mayStore = 1;
-  string Constraints = "$rt = $dst";
   string DecoderMethod = "DecodeMemEVA";
   InstrItinClass Itinerary = itin;
 }
 
 class SCE_DESC : SCE_DESC_BASE<"sce", GPR32Opnd, II_SCE>;
 
 class TLB_DESC_BASE<string instr_asm, InstrItinClass itin = NoItinerary> {
   dag OutOperandList = (outs);

lib/Target/Mips/MipsExpandPseudo.cpp

BuildMI(loop1MBB, DL, TII->get(Mips::AND), Scratch2)
       .addReg(Scratch)
       .addReg(Mask);
   BuildMI(loop1MBB, DL, TII->get(BNE))
     .addReg(Scratch2).addReg(ShiftCmpVal).addMBB(sinkMBB);
 
   // loop2MBB:
   //   and dest, dest, mask2
   //   or dest, dest, ShiftNewVal
-  //   sc dest, dest, 0(ptr)
+  //   sc dest, 0(ptr)
   //   beq dest, $0, loop1MBB
   BuildMI(loop2MBB, DL, TII->get(Mips::AND), Scratch)
       .addReg(Scratch, RegState::Kill)
       .addReg(Mask2);
   BuildMI(loop2MBB, DL, TII->get(Mips::OR), Scratch)
       .addReg(Scratch, RegState::Kill)
       .addReg(ShiftNewVal);
-  BuildMI(loop2MBB, DL, TII->get(SC), Scratch)
-      .addReg(Scratch, RegState::Kill)
+  BuildMI(loop2MBB, DL, TII->get(SC))
+      .addReg(Scratch)
       .addReg(Ptr)
       .addImm(0);
   BuildMI(loop2MBB, DL, TII->get(BEQ))
       .addReg(Scratch, RegState::Kill)
       .addReg(ZERO)
       .addMBB(loop1MBB);
 
   //  sinkMBB:
bool MipsExpandPseudo::expandAtomicCmpSwap(MachineBasicBlock &BB,
   //   ll dest, 0(ptr)
   //   bne dest, oldval, exitMBB
   BuildMI(loop1MBB, DL, TII->get(LL), Dest).addReg(Ptr).addImm(0);
   BuildMI(loop1MBB, DL, TII->get(BNE))
     .addReg(Dest, RegState::Kill).addReg(OldVal).addMBB(exitMBB);
 
   // loop2MBB:
   //   move scratch, NewVal
-  //   sc Scratch, Scratch, 0(ptr)
+  //   sc Scratch, 0(ptr)
   //   beq Scratch, $0, loop1MBB
   BuildMI(loop2MBB, DL, TII->get(MOVE), Scratch).addReg(NewVal).addReg(ZERO);
-  BuildMI(loop2MBB, DL, TII->get(SC), Scratch)
+  BuildMI(loop2MBB, DL, TII->get(SC))
     .addReg(Scratch).addReg(Ptr).addImm(0);
   BuildMI(loop2MBB, DL, TII->get(BEQ))
     .addReg(Scratch, RegState::Kill).addReg(ZERO).addMBB(loop1MBB);
 
   LivePhysRegs LiveRegs;
   computeAndAddLiveIns(LiveRegs, *loop1MBB);
   computeAndAddLiveIns(LiveRegs, *loop2MBB);
   computeAndAddLiveIns(LiveRegs, *exitMBB);
if (IsNand) {
     //  and newval, incr2, mask
     BuildMI(loopMBB, DL, TII->get(Mips::AND), BinOpRes)
         .addReg(Incr)
         .addReg(Mask);
   }
 
   // and StoreVal, OlddVal, Mask2
   // or StoreVal, StoreVal, BinOpRes
-  // StoreVal<tied1> = sc StoreVal, 0(Ptr)
+  // sc StoreVal, 0(Ptr)
   // beq StoreVal, zero, loopMBB
   BuildMI(loopMBB, DL, TII->get(Mips::AND), StoreVal)
     .addReg(OldVal).addReg(Mask2);
   BuildMI(loopMBB, DL, TII->get(Mips::OR), StoreVal)
     .addReg(StoreVal).addReg(BinOpRes);
-  BuildMI(loopMBB, DL, TII->get(SC), StoreVal)
+  BuildMI(loopMBB, DL, TII->get(SC))
     .addReg(StoreVal).addReg(Ptr).addImm(0);
   BuildMI(loopMBB, DL, TII->get(BEQ))
     .addReg(StoreVal).addReg(Mips::ZERO).addMBB(loopMBB);
 
   //  sinkMBB:
   //    and     maskedoldval1,oldval,mask
   //    srl     srlres,maskedoldval1,shiftamt
   //    sign_extend dest,srlres
assert(AND && NOR &&
            "Unknown nand instruction for atomic pseudo expansion");
     BuildMI(loopMBB, DL, TII->get(AND), Scratch).addReg(OldVal).addReg(Incr);
     BuildMI(loopMBB, DL, TII->get(NOR), Scratch).addReg(ZERO).addReg(Scratch);
   } else {
     assert(OR && "Unknown instruction for atomic pseudo expansion!");
     BuildMI(loopMBB, DL, TII->get(OR), Scratch).addReg(Incr).addReg(ZERO);
   }
 
-  BuildMI(loopMBB, DL, TII->get(SC), Scratch).addReg(Scratch).addReg(Ptr).addImm(0);
+  BuildMI(loopMBB, DL, TII->get(SC)).addReg(Scratch).addReg(Ptr).addImm(0);
   BuildMI(loopMBB, DL, TII->get(BEQ)).addReg(Scratch).addReg(ZERO).addMBB(loopMBB);
 
   NMBBI = BB.end();
   I->eraseFromParent();
 
   LivePhysRegs LiveRegs;
   computeAndAddLiveIns(LiveRegs, *loopMBB);
   computeAndAddLiveIns(LiveRegs, *exitMBB);

lib/Target/Mips/MipsInstrInfo.td

 class LLBase<string opstr, RegisterOperand RO, DAGOperand MO = mem> :
   InstSE<(outs RO:$rt), (ins MO:$addr), !strconcat(opstr, "\t$rt, $addr"),
          [], II_LL, FrmI, opstr> {
   let DecoderMethod = "DecodeMem";
   let mayLoad = 1;
 }
 
 class SCBase<string opstr, RegisterOperand RO> :
-  InstSE<(outs RO:$dst), (ins RO:$rt, mem:$addr),
+  InstSE<(outs), (ins RO:$rt, mem:$addr),
          !strconcat(opstr, "\t$rt, $addr"), [], II_SC, FrmI> {
   let DecoderMethod = "DecodeMem";
   let mayStore = 1;
-  let Constraints = "$rt = $dst";
 }
 
 class MFC3OP<string asmstr, RegisterOperand RO, RegisterOperand RD,
              InstrItinClass itin> :
   InstSE<(outs RO:$rt), (ins RD:$rd, uimm3:$sel),
          !strconcat(asmstr, "\t$rt, $rd, $sel"), [], itin, FrmFR> {
   let BaseOpcode = asmstr;
 }

test/CodeGen/Mips/micromips-eva.mir

bb.0.entry:
     SYNC 0
     %3:gpr32 = ADDiu $zero, 42
 
   bb.1.entry:
     successors: %bb.1(0x40000000), %bb.2(0x40000000)
 
     %4:gpr32 = LLE %2, 0
     %6:gpr32 = ADDu %4, %3
-    %8:gpr32 = SCE %6, %2, 0
-    BEQ %8, $zero, %bb.1, implicit-def $at
+    SCE %6, %2, 0
+    BEQ %6, $zero, %bb.1, implicit-def $at
 
   bb.2.entry:
     SYNC 0
     %5:gpr32 = ADDiu killed %4, 42
     $v0 = COPY %5
     CACHEE %1, 5, 2
     PREFE %1, 5, 2
     RetRA implicit $v0

test/MC/Mips/sym-sc.s

+# RUN: llvm-mc -filetype=obj -triple mips -mcpu=mips2 %s -o - \
+# RUN:   | llvm-objdump -d -r - | FileCheck %s --check-prefix=MIPS
+# RUN: llvm-mc -filetype=obj -triple mips -mcpu=mips32 %s -o - \
+# RUN:   | llvm-objdump -d -r - | FileCheck %s --check-prefix=MIPS
+# RUN: llvm-mc -filetype=obj -triple mips -mcpu=mips32r2 %s -o - \
+# RUN:   | llvm-objdump -d -r - | FileCheck %s --check-prefix=MIPS
+# RUN: llvm-mc -filetype=obj -triple mips -mcpu=mips3 %s -o - \
+# RUN:   | llvm-objdump -d -r - | FileCheck %s --check-prefix=MIPS
+# RUN: llvm-mc -filetype=obj -triple mips -mcpu=mips64 %s -o - \
+# RUN:   | llvm-objdump -d -r - | FileCheck %s --check-prefix=MIPS
+# RUN: llvm-mc -filetype=obj -triple mips -mcpu=mips64r2 %s -o - \
+# RUN:   | llvm-objdump -d -r - | FileCheck %s --check-prefix=MIPS
+# RUN: llvm-mc -filetype=obj -triple mips -mcpu=mips32r6 %s -o - \
+# RUN:   | llvm-objdump -d -r - | FileCheck %s --check-prefix=MIPSR6
+# RUN: llvm-mc -filetype=obj -triple mips -mcpu=mips64r6 %s -o - \
+# RUN:   | llvm-objdump -d -r - | FileCheck %s --check-prefix=MIPSR6
+# RUN: llvm-mc -filetype=obj -triple mips -mcpu=mips32r2 -mattr=+micromips %s -o - \
+# RUN:   | llvm-objdump -d -r - | FileCheck %s --check-prefix=MICROMIPSR2
+# RUN: llvm-mc -filetype=obj -triple mips -mcpu=mips32r6 -mattr=+micromips %s -o - \
+# RUN:   | llvm-objdump -d -r - | FileCheck %s --check-prefix=MICROMIPSR6
+
+# MIPS:         0:  e0 6c 00 00    sc   $12, 0($3)
+# MIPSR6:       0:  7c 6c 00 26    sc   $12, 0($3)
+# MICROMIPS:    0:  61 83 b0 00    sc   $12, 0($3)
+sc $12, 0($3)
+
+# MIPS:         4:  e0 6c 00 04    sc   $12, 4($3)
+# MIPSR6:       4:  7c 6c 02 26    sc   $12, 4($3)
+# MICROMIPS:    4:  61 83 b0 04    sc   $12, 4($3)
+sc $12, 4($3)
+
+# MIPS:         8:  3c 01 00 00    lui  $1, 0
+# MIPS:         00000008:  R_MIPS_HI16  some_random_symbol_name
+# MIPS:         c:  e0 2c 00 00    sc   $12, 0($1)
+# MIPS:         0000000c:  R_MIPS_LO16  some_random_symbol_name
+
+# MIPSR6:       8:  3c 01 00 00    aui  $1, $zero, 0
+# MIPSR6:       00000008:  R_MIPS_HI16  some_random_symbol_name
+# MIPSR6:       c:  7c 2c 00 26    sc   $12, 0($1)
+# MIPSR6:       0000000c:  R_MIPS_LO16  some_random_symbol_name
+
+# MICROMIPSR2:  8:  41 a1 00 00    lui  $1, 0
+# MICROMIPSR2:  00000008:  R_MICROMIPS_HI16  some_random_symbol_name
+# MICROMIPSR2:  c:  61 81 b0 00    sc   $12, 0($1)
+# MICROMIPSR2:  0000000c:  R_MICROMIPS_LO16  some_random_symbol_name
+
+# MICROMIPSR6:  8:  3c 01 00 00    lh   $zero, 0($1)
+# MICROMIPSR6:  00000008:  R_MICROMIPS_HI16  some_random_symbol_name
+# MICROMIPSR6:  c:  61 81 b0 00    sc   $12, 0($1)
+# MICROMIPSR6:  0000000c:  R_MICROMIPS_LO16  some_random_symbol_name
+sc $12, some_random_symbol_name