[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

51 ↗

(On Diff #211524)

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

llvm/trunk/lib/Target/Mips/AsmParser/MipsAsmParser.cpp

 }
 
 void MipsAsmParser::expandMemInst(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
                                   const MCSubtargetInfo *STI, bool IsLoad) {
   const MCOperand &DstRegOp = Inst.getOperand(0);
   assert(DstRegOp.isReg() && "expected register operand kind");
   const MCOperand &BaseRegOp = Inst.getOperand(1);
   assert(BaseRegOp.isReg() && "expected register operand kind");
-  const MCOperand &OffsetOp = Inst.getOperand(2);
 
   MipsTargetStreamer &TOut = getTargetStreamer();
   unsigned DstReg = DstRegOp.getReg();
   unsigned BaseReg = BaseRegOp.getReg();
   unsigned TmpReg = DstReg;
 
   const MCInstrDesc &Desc = getInstDesc(Inst.getOpcode());
   int16_t DstRegClass = Desc.OpInfo[0].RegClass;
   unsigned DstRegClassID =
       getContext().getRegisterInfo()->getRegClass(DstRegClass).getID();
   bool IsGPR = (DstRegClassID == Mips::GPR32RegClassID) ||
                (DstRegClassID == Mips::GPR64RegClassID);
 
   if (!IsLoad || !IsGPR || (BaseReg == DstReg)) {
     // At this point we need AT to perform the expansions
     // and we exit if it is not available.
     TmpReg = getATReg(IDLoc);
     if (!TmpReg)
       return;
   }
 
+  if (Inst.getNumOperands() > 3) {
+    const MCOperand &BaseRegOp = Inst.getOperand(2);
+    assert(BaseRegOp.isReg() && "expected register operand kind");
+    const MCOperand &ExprOp = Inst.getOperand(3);
+    assert(ExprOp.isExpr() && "expected expression oprand kind");
+
+    unsigned BaseReg = BaseRegOp.getReg();
+    const MCExpr *ExprOffset = ExprOp.getExpr();
+
+    MCOperand LoOperand = MCOperand::createExpr(
+        MipsMCExpr::create(MipsMCExpr::MEK_LO, ExprOffset, getContext()));
+    MCOperand HiOperand = MCOperand::createExpr(
+        MipsMCExpr::create(MipsMCExpr::MEK_HI, ExprOffset, getContext()));
+    TOut.emitSCWithSymOffset(Inst.getOpcode(), DstReg, BaseReg, HiOperand,
+                             LoOperand, TmpReg, IDLoc, STI);
+    return;
+  }
+
+  const MCOperand &OffsetOp = Inst.getOperand(2);
+
   if (OffsetOp.isImm()) {
     int64_t LoOffset = OffsetOp.getImm() & 0xffff;
     int64_t HiOffset = OffsetOp.getImm() & ~0xffff;
 
     // If msb of LoOffset is 1(negative number) we must increment
     // HiOffset to account for the sign-extension of the low part.
     if (LoOffset & 0x8000)
       HiOffset += 0x10000;
if (OffsetOp.isImm()) {
 
     if (BaseReg != Mips::ZERO && BaseReg != Mips::ZERO_64)
       TOut.emitRRR(isGP64bit() ? Mips::DADDu : Mips::ADDu, TmpReg, TmpReg,
                    BaseReg, IDLoc, STI);
     TOut.emitRRI(Inst.getOpcode(), DstReg, TmpReg, LoOffset, IDLoc, STI);
     return;
   }
 
-  assert(OffsetOp.isExpr() && "expected expression operand kind");
+  if (OffsetOp.isExpr()) {
     if (inPicMode()) {
       // FIXME:
       // a) Fix lw/sw $reg, symbol($reg) instruction expanding.
       // b) If expression includes offset (sym + number), do not
       //    encode the offset into a relocation. Take it in account
       //    in the last load/store instruction.
       // c) Check that immediates of R_MIPS_GOT16/R_MIPS_LO16 relocations
       //    do not exceed 16-bit.
       // d) Use R_MIPS_GOT_PAGE/R_MIPS_GOT_OFST relocations instead
       //    of R_MIPS_GOT_DISP in appropriate cases to reduce number
       //    of GOT entries.
       expandLoadAddress(TmpReg, Mips::NoRegister, OffsetOp, !ABI.ArePtrs64bit(),
                         IDLoc, Out, STI);
       TOut.emitRRI(Inst.getOpcode(), DstReg, TmpReg, 0, IDLoc, STI);
     } else {
       const MCExpr *ExprOffset = OffsetOp.getExpr();
       MCOperand LoOperand = MCOperand::createExpr(
           MipsMCExpr::create(MipsMCExpr::MEK_LO, ExprOffset, getContext()));
       MCOperand HiOperand = MCOperand::createExpr(
           MipsMCExpr::create(MipsMCExpr::MEK_HI, ExprOffset, getContext()));
 
       if (IsLoad)
         TOut.emitLoadWithSymOffset(Inst.getOpcode(), DstReg, BaseReg, HiOperand,
                                    LoOperand, TmpReg, IDLoc, STI);
       else
-      TOut.emitStoreWithSymOffset(Inst.getOpcode(), DstReg, BaseReg, HiOperand,
-                                  LoOperand, TmpReg, IDLoc, STI);
+        TOut.emitStoreWithSymOffset(Inst.getOpcode(), DstReg, BaseReg,
+                                    HiOperand, LoOperand, TmpReg, IDLoc, STI);
+    }
+    return;
   }
+
+  llvm_unreachable("unexpected operand type");
 }
 
 bool MipsAsmParser::expandLoadStoreMultiple(MCInst &Inst, SMLoc IDLoc,
                                             MCStreamer &Out,
                                             const MCSubtargetInfo *STI) {
   unsigned OpNum = Inst.getNumOperands();
   unsigned Opcode = Inst.getOpcode();
   unsigned NewOpcode = Opcode == Mips::SWM_MM ? Mips::SWM32_MM : Mips::LWM32_MM;

llvm/trunk/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp

 }
 
 void MipsTargetStreamer::emitRRR(unsigned Opcode, unsigned Reg0, unsigned Reg1,
                                  unsigned Reg2, SMLoc IDLoc,
                                  const MCSubtargetInfo *STI) {
   emitRRX(Opcode, Reg0, Reg1, MCOperand::createReg(Reg2), IDLoc, STI);
 }
 
+void MipsTargetStreamer::emitRRRX(unsigned Opcode, unsigned Reg0, unsigned Reg1,
+                                  unsigned Reg2, MCOperand Op3, SMLoc IDLoc,
+                                  const MCSubtargetInfo *STI) {
+  MCInst TmpInst;
+  TmpInst.setOpcode(Opcode);
+  TmpInst.addOperand(MCOperand::createReg(Reg0));
+  TmpInst.addOperand(MCOperand::createReg(Reg1));
+  TmpInst.addOperand(MCOperand::createReg(Reg2));
+  TmpInst.addOperand(Op3);
+  TmpInst.setLoc(IDLoc);
+  getStreamer().EmitInstruction(TmpInst, *STI);
+}
+
 void MipsTargetStreamer::emitRRI(unsigned Opcode, unsigned Reg0, unsigned Reg1,
                                  int16_t Imm, SMLoc IDLoc,
                                  const MCSubtargetInfo *STI) {
   emitRRX(Opcode, Reg0, Reg1, MCOperand::createImm(Imm), IDLoc, STI);
 }
 
 void MipsTargetStreamer::emitRRIII(unsigned Opcode, unsigned Reg0,
                                    unsigned Reg1, int16_t Imm0, int16_t Imm1,
void MipsTargetStreamer::emitStoreWithSymOffset(
   // Generate the base address in ATReg.
   emitRX(Mips::LUi, ATReg, HiOperand, IDLoc, STI);
   if (BaseReg != Mips::ZERO)
     emitRRR(Mips::ADDu, ATReg, ATReg, BaseReg, IDLoc, STI);
   // Emit the store with the adjusted base and offset.
   emitRRX(Opcode, SrcReg, ATReg, LoOperand, IDLoc, STI);
 }
 
+/// Emit a store instruction with an symbol offset.
+void MipsTargetStreamer::emitSCWithSymOffset(unsigned Opcode, unsigned SrcReg,
+                                             unsigned BaseReg,
+                                             MCOperand &HiOperand,
+                                             MCOperand &LoOperand,
+                                             unsigned ATReg, SMLoc IDLoc,
+                                             const MCSubtargetInfo *STI) {
+  // sc $8, sym => lui $at, %hi(sym)
+  //               sc $8, %lo(sym)($at)
+
+  // Generate the base address in ATReg.
+  emitRX(Mips::LUi, ATReg, HiOperand, IDLoc, STI);
+  if (!isMicroMips(STI) && isMipsR6(STI)) {
+    // For non-micromips r6 offset for 'sc' is not in the lower 16 bits so we
+    // put it in 'at'.
+    // sc $8, sym => lui $at, %hi(sym)
+    //               addiu $at, $at, %lo(sym)
+    //               sc $8, 0($at)
+    emitRRX(Mips::ADDiu, ATReg, ATReg, LoOperand, IDLoc, STI);
+    MCOperand Offset = MCOperand::createImm(0);
+    // Emit the store with the adjusted base and offset.
+    emitRRRX(Opcode, SrcReg, SrcReg, ATReg, Offset, IDLoc, STI);
+  } else {
+    if (BaseReg != Mips::ZERO)
+      emitRRR(Mips::ADDu, ATReg, ATReg, BaseReg, IDLoc, STI);
+    // Emit the store with the adjusted base and offset.
+    emitRRRX(Opcode, SrcReg, SrcReg, ATReg, LoOperand, IDLoc, STI);
+  }
+}
+
 /// Emit a load instruction with an immediate offset. DstReg and TmpReg are
 /// permitted to be the same register iff DstReg is distinct from BaseReg and
 /// DstReg is a GPR. It is the callers responsibility to identify such cases
 /// and pass the appropriate register in TmpReg.
 void MipsTargetStreamer::emitLoadWithImmOffset(unsigned Opcode, unsigned DstReg,
                                                unsigned BaseReg, int64_t Offset,
                                                unsigned TmpReg, SMLoc IDLoc,
                                                const MCSubtargetInfo *STI) {
void MipsTargetStreamer::emitLoadWithSymOffset(unsigned Opcode, unsigned DstReg,
   // Generate the base address in TmpReg.
   emitRX(Mips::LUi, TmpReg, HiOperand, IDLoc, STI);
   if (BaseReg != Mips::ZERO)
     emitRRR(Mips::ADDu, TmpReg, TmpReg, BaseReg, IDLoc, STI);
   // Emit the load with the adjusted base and offset.
   emitRRX(Opcode, DstReg, TmpReg, LoOperand, IDLoc, STI);
 }
 
+bool MipsTargetStreamer::isMipsR6(const MCSubtargetInfo *STI) const {
+  return STI->getFeatureBits()[Mips::FeatureMips32r6] ||
+         STI->getFeatureBits()[Mips::FeatureMips64r6];
+}
+
+bool MipsTargetStreamer::isMicroMips(const MCSubtargetInfo *STI) const {
+  return STI->getFeatureBits()[Mips::FeatureMicroMips];
+}
+
 MipsTargetAsmStreamer::MipsTargetAsmStreamer(MCStreamer &S,
                                              formatted_raw_ostream &OS)
     : MipsTargetStreamer(S), OS(OS) {}
 
 void MipsTargetAsmStreamer::emitDirectiveSetMicroMips() {
   OS << "\t.set\tmicromips\n";
   forbidModuleDirective();
 }

llvm/trunk/lib/Target/Mips/MipsTargetStreamer.h

public:
   void emitRI(unsigned Opcode, unsigned Reg0, int32_t Imm, SMLoc IDLoc,
               const MCSubtargetInfo *STI);
   void emitRR(unsigned Opcode, unsigned Reg0, unsigned Reg1, SMLoc IDLoc,
               const MCSubtargetInfo *STI);
   void emitRRX(unsigned Opcode, unsigned Reg0, unsigned Reg1, MCOperand Op2,
                SMLoc IDLoc, const MCSubtargetInfo *STI);
   void emitRRR(unsigned Opcode, unsigned Reg0, unsigned Reg1, unsigned Reg2,
                SMLoc IDLoc, const MCSubtargetInfo *STI);
+  void emitRRRX(unsigned Opcode, unsigned Reg0, unsigned Reg1, unsigned Reg2,
+                MCOperand Op3, SMLoc IDLoc, const MCSubtargetInfo *STI);
   void emitRRI(unsigned Opcode, unsigned Reg0, unsigned Reg1, int16_t Imm,
                SMLoc IDLoc, const MCSubtargetInfo *STI);
   void emitRRIII(unsigned Opcode, unsigned Reg0, unsigned Reg1, int16_t Imm0,
                  int16_t Imm1, int16_t Imm2, SMLoc IDLoc,
                  const MCSubtargetInfo *STI);
   void emitAddu(unsigned DstReg, unsigned SrcReg, unsigned TrgReg, bool Is64Bit,
                 const MCSubtargetInfo *STI);
   void emitDSLL(unsigned DstReg, unsigned SrcReg, int16_t ShiftAmount,
public:
   void emitStoreWithImmOffset(unsigned Opcode, unsigned SrcReg,
                               unsigned BaseReg, int64_t Offset,
                               function_ref<unsigned()> GetATReg, SMLoc IDLoc,
                               const MCSubtargetInfo *STI);
   void emitStoreWithSymOffset(unsigned Opcode, unsigned SrcReg,
                               unsigned BaseReg, MCOperand &HiOperand,
                               MCOperand &LoOperand, unsigned ATReg, SMLoc IDLoc,
                               const MCSubtargetInfo *STI);
+  void emitSCWithSymOffset(unsigned Opcode, unsigned SrcReg, unsigned BaseReg,
+                           MCOperand &HiOperand, MCOperand &LoOperand,
+                           unsigned ATReg, SMLoc IDLoc,
+                           const MCSubtargetInfo *STI);
   void emitLoadWithImmOffset(unsigned Opcode, unsigned DstReg, unsigned BaseReg,
                              int64_t Offset, unsigned TmpReg, SMLoc IDLoc,
                              const MCSubtargetInfo *STI);
   void emitLoadWithSymOffset(unsigned Opcode, unsigned DstReg, unsigned BaseReg,
                              MCOperand &HiOperand, MCOperand &LoOperand,
                              unsigned ATReg, SMLoc IDLoc,
                              const MCSubtargetInfo *STI);
   void emitGPRestore(int Offset, SMLoc IDLoc, const MCSubtargetInfo *STI);
public:
   }
 
   MipsABIFlagsSection &getABIFlagsSection() { return ABIFlagsSection; }
   const MipsABIInfo &getABI() const {
     assert(ABI.hasValue() && "ABI hasn't been set!");
     return *ABI;
   }
 
+  bool isMipsR6(const MCSubtargetInfo *STI) const;
+  bool isMicroMips(const MCSubtargetInfo *STI) const;
+
 protected:
   llvm::Optional<MipsABIInfo> ABI;
   MipsABIFlagsSection ABIFlagsSection;
 
   bool GPRInfoSet;
   unsigned GPRBitMask;
   int GPROffset;
 

llvm/trunk/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-prefixes=MICROMIPS,MICROMIPSR2
+# RUN: llvm-mc -filetype=obj -triple mips -mcpu=mips32r6 -mattr=+micromips %s -o - \
+# RUN:   | llvm-objdump -d -r - | FileCheck %s --check-prefixes=MICROMIPS,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  symbol
+# MIPS:          c:  e0 2c 00 00    sc   $12, 0($1)
+# MIPS:         0000000c:  R_MIPS_LO16  symbol
+
+# MIPSR6:        8: 3c 01 00 00     aui    $1, $zero, 0
+# MIPSR6:       00000008:  R_MIPS_HI16  symbol
+# MIPSR6:        c: 24 21 00 00     addiu  $1, $1, 0
+# MIPSR6:       0000000c:  R_MIPS_LO16  symbol
+# MIPSR6:       10: 7c 2c 00 26     sc     $12, 0($1)
+
+# MICROMIPSR2:   8:  41 a1 00 00    lui  $1, 0
+# MICROMIPSR2:  00000008:  R_MICROMIPS_HI16  symbol
+# MICROMIPSR2:   c:  61 81 b0 00    sc   $12, 0($1)
+# MICROMIPSR2:  0000000c:  R_MICROMIPS_LO16  symbol
+
+# MICROMIPSR6:   8:  3c 01 00 00    lh   $zero, 0($1)
+# MICROMIPSR6:  00000008:  R_MICROMIPS_HI16  symbol
+# MICROMIPSR6:   c:  61 81 b0 00    sc   $12, 0($1)
+# MICROMIPSR6:  0000000c:  R_MICROMIPS_LO16  symbol
+sc $12, symbol
+
+# MIPS:         10:  3c 01 00 00    lui  $1, 0
+# MIPS:         00000010:  R_MIPS_HI16  symbol
+# MIPS:         14:  e0 2c 00 08    sc   $12, 8($1)
+# MIPS:         00000014:  R_MIPS_LO16  symbol
+
+# MIPSR6:       14: 3c 01 00 00     aui    $1, $zero, 0
+# MIPSR6:       00000014:  R_MIPS_HI16  symbol
+# MIPSR6:       18: 24 21 00 08     addiu  $1, $1, 8
+# MIPSR6:       00000018:  R_MIPS_LO16  symbol
+# MIPSR6:       1c: 7c 2c 00 26     sc     $12, 0($1)
+
+# MICROMIPSR2:  10:  41 a1 00 00    lui  $1, 0
+# MICROMIPSR2:  00000010:  R_MICROMIPS_HI16  symbol
+# MICROMIPSR2:  14:  61 81 b0 08    sc   $12, 8($1)
+# MICROMIPSR2:  00000014:  R_MICROMIPS_LO16  symbol
+
+# MICROMIPSR6:  10:  3c 01 00 00    lh   $zero, 0($1)
+# MICROMIPSR6:  00000010:  R_MICROMIPS_HI16  symbol
+# MICROMIPSR6:  14:  61 81 b0 08    sc   $12, 8($1)
+# MICROMIPSR6:  00000014:  R_MICROMIPS_LO16  symbol
+sc $12, symbol + 8