[mips] [IAS] Add support for the .insn directive.

[tomatabacu]

Summary

This assembler directive marks the current label as an instruction label in microMIPS and MIPS16.

This initial implementation works only for microMIPS.

[tomatabacu]

Ping.

[tomatabacu]

Ping.

[dsanders]

LGTM with a small change.

lib/Target/Mips/MCTargetDesc/MipsELFStreamer.cpp

53–54

We should unconditionally clear Labels as the previous code did. We don't need to mark the same labels multiple times.

[tomatabacu]

Replied to review inline comment.

lib/Target/Mips/MCTargetDesc/MipsELFStreamer.cpp

53–54

Labels actually is unconditionally cleared: in markLabelsAsMicroMips() for the if and directly for the else.

[dsanders]

lib/Target/Mips/MCTargetDesc/MipsELFStreamer.cpp

53–54

Good point. For consistency, could you pull it out of markLabelsAsMicroMips() into the two callers or sink the isMicroMipsEnabled() checks inside the markLabelsAsMicroMips() as an early-exit? Both options make the usage consistent and neither is better than the other. If you choose the latter though, you ought to change the name accordingly since it won't touch the labels if micromips is not enabled.

lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp

649–652

I've just noticed that the Labels.clear() is missing here.

[tomatabacu]

Replied to inline review comments.

lib/Target/Mips/MCTargetDesc/MipsELFStreamer.cpp

53–54

"could you pull it out of markLabelsAsMicroMips() into the two callers"
No, because Labels can not be directly accessed from MipsTargetELFStreamer::emitDirectiveInsn(), because it's a private member of MipsELFStreamer.

"or sink the isMicroMipsEnabled() checks inside the markLabelsAsMicroMips() as an early-exit?"
I would personally prefer conditionally calling a function that will execute rather than unconditionally calling a function that may or may not execute.
Also, I can't think of a name for markLabelsAsMicroMips() with the microMIPS check inside which has a reasonable length.

lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp

649–652

It's not missing, it's in markLabelsAsMicroMips(). We can't directly access Labels from here, because it's a private member of MipsELFStreamer.

[dsanders]

lib/Target/Mips/MCTargetDesc/MipsELFStreamer.cpp

53–54

"or sink the isMicroMipsEnabled() checks inside the markLabelsAsMicroMips() as an early-exit?"

I would personally prefer conditionally calling a function that will execute rather than unconditionally calling a function that may or may not execute.

There's nothing particularly wrong with a function doing nothing when it has nothing to do. In this case it wouldn't quite do nothing though. It would still empty the list of labels regardless of whether it annotated the labels or not.

Also, I can't think of a name for markLabelsAsMicroMips() with the microMIPS check inside which has a reasonable length.

The best name I can think of is createPendingLabelRelocs().

lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp

649–652

It's not missing, it's in markLabelsAsMicroMips().

I mean it's missing from the else path. Sorry for being unclear.

The case I have in mind is something like:

.set nomicromips
foo:
  .insn
  .4byte 0
.set micromips
bar:
  addiu $1, $2, 3

I think the addiu will cause foo and bar to be marked microMIPS when it should only mark bar. This is because because the .insn didn't clear the list.

We can't directly access Labels from here, because it's a private member of MipsELFStreamer.

A public member for clearing the labels list would fix that but we won't need that if markLabelsAsMicroMips is called unconditionally and always clears the list regardless of whether it marks the labels or not.

[tomatabacu]

Renamed markLabelsAsMicroMips() to createPendingLabelRelocs().
Moved microMIPS check inside createPendingLabelRelocs().
Unconditionally clear Labels inside createPendingLabelRelocs().
Updated test to check that the labels get cleared after .insn.

[dsanders]

LGTM with a couple tiny nits. Thanks.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

4059

Nit: There's usually '()' when referencing functions.

lib/Target/Mips/MCTargetDesc/MipsELFStreamer.h

69

Nit: ... if necessary for the subtarget.

[tomatabacu]

Addressed nits.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

#include "MipsGenAsmMatcher.inc"
   bool parseSetMips0Directive();
   bool parseSetArchDirective();
   bool parseSetFeature(uint64_t Feature);
   bool parseDirectiveCpLoad(SMLoc Loc);
   bool parseDirectiveCPSetup();
   bool parseDirectiveNaN();
   bool parseDirectiveSet();
   bool parseDirectiveOption();
+  bool parseInsnDirective();
 
   bool parseSetAtDirective();
   bool parseSetNoAtDirective();
   bool parseSetMacroDirective();
   bool parseSetNoMacroDirective();
   bool parseSetMsaDirective();
   bool parseSetNoMsaDirective();
   bool parseSetNoDspDirective();
bool MipsAsmParser::parseDirectiveOption() {
 
   // Unknown option.
   Warning(Parser.getTok().getLoc(), 
           "unknown option, expected 'pic0' or 'pic2'");
   Parser.eatToEndOfStatement();
   return false;
 }
 
+/// parseInsnDirective
+///  ::= .insn
+bool MipsAsmParser::parseInsnDirective() {
+  // If this is not the end of the statement, report an error.
+  if (getLexer().isNot(AsmToken::EndOfStatement)) {
+    reportParseError("unexpected token, expected end of statement");
+    return false;
+  }
+
+  // The actual label marking happens in
+  // MipsELFStreamer::createPendingLabelRelocs().
+  getTargetStreamer().emitDirectiveInsn();
+
+  getParser().Lex(); // Eat EndOfStatement token.
+  return false;
+}
+
 /// parseDirectiveModule
 ///  ::= .module oddspreg
 ///  ::= .module nooddspreg
 ///  ::= .module fp=value
 bool MipsAsmParser::parseDirectiveModule() {
   MCAsmParser &Parser = getParser();
   MCAsmLexer &Lexer = getLexer();
   SMLoc L = Lexer.getLoc();
if (IDVal == ".cpsetup")
     return parseDirectiveCPSetup();
 
   if (IDVal == ".module")
     return parseDirectiveModule();
 
   if (IDVal == ".llvm_internal_mips_reallow_module_directive")
     return parseInternalDirectiveReallowModule();
 
+  if (IDVal == ".insn")
+    return parseInsnDirective();
+
   return true;
 }
 
 bool MipsAsmParser::parseInternalDirectiveReallowModule() {
   // If this is not the end of the statement, report an error.
   if (getLexer().isNot(AsmToken::EndOfStatement)) {
     reportParseError("unexpected token, expected end of statement");
     return false;

lib/Target/Mips/MCTargetDesc/MipsELFStreamer.h

public:
 
   /// Overriding this function allows us to dismiss all labels that are
   /// candidates for marking as microMIPS when .word directive is emitted.
   void EmitValueImpl(const MCExpr *Value, unsigned Size,
                      const SMLoc &Loc) override;
 
   /// Emits all the option records stored up until the point it's called.
   void EmitMipsOptionRecords();
+
+  /// Mark labels as microMIPS, if necessary for the subtarget.
+  void createPendingLabelRelocs();
 };
 
 MCELFStreamer *createMipsELFStreamer(MCContext &Context, MCAsmBackend &MAB,
                                      raw_pwrite_stream &OS,
                                      MCCodeEmitter *Emitter, bool RelaxAll);
 } // namespace llvm.
 #endif

lib/Target/Mips/MCTargetDesc/MipsELFStreamer.cpp

 using namespace llvm;
 
 void MipsELFStreamer::EmitInstruction(const MCInst &Inst,
                                       const MCSubtargetInfo &STI) {
   MCELFStreamer::EmitInstruction(Inst, STI);
 
   MCContext &Context = getContext();
   const MCRegisterInfo *MCRegInfo = Context.getRegisterInfo();
-  MipsTargetELFStreamer *ELFTargetStreamer =
-      static_cast<MipsTargetELFStreamer *>(getTargetStreamer());
 
   for (unsigned OpIndex = 0; OpIndex < Inst.getNumOperands(); ++OpIndex) {
     const MCOperand &Op = Inst.getOperand(OpIndex);
 
     if (!Op.isReg())
       continue;
 
     unsigned Reg = Op.getReg();
     RegInfoRecord->SetPhysRegUsed(Reg, MCRegInfo);
   }
 
+  createPendingLabelRelocs();
+}
+
+void MipsELFStreamer::createPendingLabelRelocs() {
+  MipsTargetELFStreamer *ELFTargetStreamer =
+      static_cast<MipsTargetELFStreamer *>(getTargetStreamer());
+
+  // FIXME: Also mark labels when in MIPS16 mode.
   if (ELFTargetStreamer->isMicroMipsEnabled()) {
     for (auto Label : Labels) {
       MCSymbolData &Data = getOrCreateSymbolData(Label);
       // The "other" values are stored in the last 6 bits of the second byte.
       // The traditional defines for STO values assume the full byte and thus
       // the shift to pack it.
       MCELF::setOther(Data, ELF::STO_MIPS_MICROMIPS >> 2);
     }
   }
 
   Labels.clear();
 }
 
 void MipsELFStreamer::EmitLabel(MCSymbol *Symbol) {
   MCELFStreamer::EmitLabel(Symbol);
   Labels.push_back(Symbol);
 }
 
 void MipsELFStreamer::SwitchSection(const MCSection * Section,
                                     const MCExpr *Subsection) {

lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp

 void MipsTargetStreamer::emitDirectiveSetNoAt() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveEnd(StringRef Name) {}
 void MipsTargetStreamer::emitDirectiveEnt(const MCSymbol &Symbol) {}
 void MipsTargetStreamer::emitDirectiveAbiCalls() {}
 void MipsTargetStreamer::emitDirectiveNaN2008() {}
 void MipsTargetStreamer::emitDirectiveNaNLegacy() {}
 void MipsTargetStreamer::emitDirectiveOptionPic0() {}
 void MipsTargetStreamer::emitDirectiveOptionPic2() {}
+void MipsTargetStreamer::emitDirectiveInsn() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitFrame(unsigned StackReg, unsigned StackSize,
                                    unsigned ReturnReg) {}
 void MipsTargetStreamer::emitMask(unsigned CPUBitmask, int CPUTopSavedRegOff) {}
 void MipsTargetStreamer::emitFMask(unsigned FPUBitmask, int FPUTopSavedRegOff) {
 }
 void MipsTargetStreamer::emitDirectiveSetArch(StringRef Arch) {
   forbidModuleDirective();
 }

 void MipsTargetAsmStreamer::emitDirectiveOptionPic0() {
   OS << "\t.option\tpic0\n";
 }
 
 void MipsTargetAsmStreamer::emitDirectiveOptionPic2() {
   OS << "\t.option\tpic2\n";
 }
 
+void MipsTargetAsmStreamer::emitDirectiveInsn() {
+  MipsTargetStreamer::emitDirectiveInsn();
+  OS << "\t.insn\n";
+}
+
 void MipsTargetAsmStreamer::emitFrame(unsigned StackReg, unsigned StackSize,
                                       unsigned ReturnReg) {
   OS << "\t.frame\t$"
      << StringRef(MipsInstPrinter::getRegisterName(StackReg)).lower() << ","
      << StackSize << ",$"
      << StringRef(MipsInstPrinter::getRegisterName(ReturnReg)).lower() << '\n';
 }
 
void MipsTargetELFStreamer::emitDirectiveOptionPic2() {
   // NOTE: We are following the GAS behaviour here which means the directive
   // 'pic2' also sets the CPIC bit in the ELF header. This is different from
   // what is stated in the SYSV ABI which consider the bits EF_MIPS_PIC and
   // EF_MIPS_CPIC to be mutually exclusive.
   Flags |= ELF::EF_MIPS_PIC | ELF::EF_MIPS_CPIC;
   MCA.setELFHeaderEFlags(Flags);
 }
 
+void MipsTargetELFStreamer::emitDirectiveInsn() {
+  MipsTargetStreamer::emitDirectiveInsn();
+  MipsELFStreamer &MEF = static_cast<MipsELFStreamer &>(Streamer);
+  MEF.createPendingLabelRelocs();
+}
+
 void MipsTargetELFStreamer::emitFrame(unsigned StackReg, unsigned StackSize,
                                       unsigned ReturnReg_) {
   MCContext &Context = getStreamer().getAssembler().getContext();
   const MCRegisterInfo *RegInfo = Context.getRegisterInfo();
 
   FrameInfoSet = true;
   FrameReg = RegInfo->getEncodingValue(StackReg);
   FrameOffset = StackSize;
   ReturnReg = RegInfo->getEncodingValue(ReturnReg_);

lib/Target/Mips/MipsTargetStreamer.h

public:
   virtual void emitDirectiveEnd(StringRef Name);
 
   virtual void emitDirectiveEnt(const MCSymbol &Symbol);
   virtual void emitDirectiveAbiCalls();
   virtual void emitDirectiveNaN2008();
   virtual void emitDirectiveNaNLegacy();
   virtual void emitDirectiveOptionPic0();
   virtual void emitDirectiveOptionPic2();
+  virtual void emitDirectiveInsn();
   virtual void emitFrame(unsigned StackReg, unsigned StackSize,
                          unsigned ReturnReg);
   virtual void emitMask(unsigned CPUBitmask, int CPUTopSavedRegOff);
   virtual void emitFMask(unsigned FPUBitmask, int FPUTopSavedRegOff);
 
   virtual void emitDirectiveSetArch(StringRef Arch);
   virtual void emitDirectiveSetMips0();
   virtual void emitDirectiveSetMips1();
public:
   void emitDirectiveEnd(StringRef Name) override;
 
   void emitDirectiveEnt(const MCSymbol &Symbol) override;
   void emitDirectiveAbiCalls() override;
   void emitDirectiveNaN2008() override;
   void emitDirectiveNaNLegacy() override;
   void emitDirectiveOptionPic0() override;
   void emitDirectiveOptionPic2() override;
+  void emitDirectiveInsn() override;
   void emitFrame(unsigned StackReg, unsigned StackSize,
                  unsigned ReturnReg) override;
   void emitMask(unsigned CPUBitmask, int CPUTopSavedRegOff) override;
   void emitFMask(unsigned FPUBitmask, int FPUTopSavedRegOff) override;
 
   void emitDirectiveSetArch(StringRef Arch) override;
   void emitDirectiveSetMips0() override;
   void emitDirectiveSetMips1() override;
public:
   void emitDirectiveEnd(StringRef Name) override;
 
   void emitDirectiveEnt(const MCSymbol &Symbol) override;
   void emitDirectiveAbiCalls() override;
   void emitDirectiveNaN2008() override;
   void emitDirectiveNaNLegacy() override;
   void emitDirectiveOptionPic0() override;
   void emitDirectiveOptionPic2() override;
+  void emitDirectiveInsn() override;
   void emitFrame(unsigned StackReg, unsigned StackSize,
                  unsigned ReturnReg) override;
   void emitMask(unsigned CPUBitmask, int CPUTopSavedRegOff) override;
   void emitFMask(unsigned FPUBitmask, int FPUTopSavedRegOff) override;
 
   // PIC support
   void emitDirectiveCpLoad(unsigned RegNo) override;
   void emitDirectiveCpsetup(unsigned RegNo, int RegOrOffset,
                             const MCSymbol &Sym, bool IsReg) override;
 
   // ABI Flags
   void emitDirectiveModuleOddSPReg(bool Enabled, bool IsO32ABI) override;
   void emitMipsAbiFlags();
 };
 }
 #endif

test/MC/Mips/insn-directive.s

+# RUN: llvm-mc %s -arch=mips -mcpu=mips32 | FileCheck %s --check-prefix=ASM
+
+# RUN: llvm-mc %s -arch=mips -mcpu=mips32 -filetype=obj -o - | \
+# RUN:   llvm-readobj -symbols - | FileCheck %s --check-prefix=OBJ
+
+  .set micromips
+
+  .global f_mm_insn_data
+  .type f_mm_insn_data, @function
+f_mm_insn_data:
+  .insn
+  .word 0x00e73910   # add $7, $7, $7
+
+  .global f_mm_insn_instr
+  .type f_mm_insn_instr, @function
+f_mm_insn_instr:
+  .insn
+  add $7, $7, $7
+
+  .global o_mm_insn_data
+  .type o_mm_insn_data, @object
+o_mm_insn_data:
+  .insn
+  .word 0x00e73910   # add $7, $7, $7
+
+  .global o_mm_insn_instr
+  .type o_mm_insn_instr, @object
+o_mm_insn_instr:
+  .insn
+  add $7, $7, $7
+
+  .set nomicromips
+
+  .global f_normal_insn_data
+  .type f_normal_insn_data, @function
+f_normal_insn_data:
+  .insn
+  .word 0x00e73820   # add $7, $7, $7
+
+  .global f_normal_insn_instr
+  .type f_normal_insn_instr, @function
+f_normal_insn_instr:
+  .insn
+  add $7, $7, $7
+
+  .global o_normal_insn_data
+  .type o_normal_insn_data, @object
+o_normal_insn_data:
+  .insn
+  .word 0x00e73820   # add $7, $7, $7
+
+  .global o_normal_insn_instr
+  .type o_normal_insn_instr, @object
+o_normal_insn_instr:
+  .insn
+  add $7, $7, $7
+
+# Verify that .insn causes the currently saved labels to be cleared by checking
+# that foo doesn't get marked.
+  .set nomicromips
+foo:
+  .insn
+  .word 0x00e73820   # add $7, $7, $7
+
+  .set micromips
+bar:
+  add $7, $7, $7
+
+# ASM: .insn
+
+# OBJ: Symbols [
+# OBJ: Name: foo
+# OBJ: Other: 0
+
+# OBJ: Name: f_mm_insn_data
+# OBJ: Other: 128
+
+# OBJ: Name: f_mm_insn_instr
+# OBJ: Other: 128
+
+# OBJ: Name: f_normal_insn_data
+# OBJ: Other: 0
+
+# OBJ: Name: f_normal_insn_instr
+# OBJ: Other: 0
+
+# OBJ: Name: o_mm_insn_data
+# OBJ: Other: 128
+
+# OBJ: Name: o_mm_insn_instr
+# OBJ: Other: 128
+
+# OBJ: Name: o_normal_insn_data
+# OBJ: Other: 0
+
+# OBJ: Name: o_normal_insn_instr
+# OBJ: Other: 0
+# OBJ: ]