[RISCV]Generate target attribute in attribute section of object file when assemble .s file

[zixuan-wu]

Summary

When there is not RISCV arch attribute in assembly .s file, it will not generate arch attribute in obj file. LLVM asm parser does not use subtarget feature bits to generate appropriate attributes. So instructions can not be recognized in disassemble process (llvm-objdump).

For example,

;t.s

fadd.s ft1,ft2,ft3

./bin/llvm-mc < t.s -mattr=+f -filetype=obj |./bin/llvm-objdump -dr -

0000000000000000 <.text>:
       0: d3 70 31 00  	<unknown>

Emit target attribute at the constructor of RISCV asm parser to enable the functionality of attribute section even in assemble process.

It's a WIP patch to discuss and see whether the direction is right.

[jrtc27]

Please upload patches using full context; see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface (or the section above it; arc diff does everything for you).

Whilst we should be emitting attributes for hand-written assembly files for anything not explicitly specified, that should only be done for the top-level file, not for inline assembly.

llvm/test/MC/RISCV/rvf-user-csr-names.s

6

So, this used to be required until D58932. I guess it's fine to remove it from the llvm-mc invocation (though this one fails to delete one of the spaces); the alternative would be to add --mattr=-f to the llvm-objdump invocation, but I think this diff makes more sense.

[zixuan-wu]

In D109587#2994117, @jrtc27 wrote:

Please upload patches using full context; see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface (or the section above it; arc diff does everything for you).

Whilst we should be emitting attributes for hand-written assembly files for anything not explicitly specified, that should only be done for the top-level file, not for inline assembly.

You mean when the asm parser is constructed only for inline-asm, then do not generate attributes instead of other code path invocation ? Is there any predict/judgement can be used in asm parser constructor to see whether it's in inline-asm code path?

[MaskRay]

For other targets, llvm-objdump is just recommended to default to something like binutils -mcpu=all (GCC powerpc -mcpu=future) if no attribute section says anything about the ISA. That may be an issue if there are conflicting encoding in one ISA.

[zixuan-wu]

In D109587#2997870, @MaskRay wrote:

For other targets, llvm-objdump is just recommended to default to something like binutils -mcpu=all (GCC powerpc -mcpu=future) if no attribute section says anything about the ISA. That may be an issue if there are conflicting encoding in one ISA.

The key point is at assembly side not objdump side. Indeed, there is conflicting encoding in one ISA, so that generate appropriate different attribute for same instruction encoding. The attributes of assemble and disassemble just should be match.

[kito-cheng]

GNU toolchain is always emit arch attribute even user didn't write it in assembly file, and it will implied the info from the -march option or default arch setting, it's what you want to do in LLVM part IIUC.

But this implementation has some issue:

• Inline asm will emit .attribute 5, (.attribute Tag_RISCV_arch) again, that result the output will reject by the GNU assembler since .attribute Tag_RISCV_arch required to be appear before any instruction.
• NO new testcase added to demonstrate what you want to do, I would suggest you should add one, assemble a .s file to .o and then dump the attribute to make sure .attribute Tag_RISCV_arch has emitted properly.

So I think the goal of this patch is right way, but the implementation need some tweaks, especially the code gen change for inline asm.

[zixuan-wu]

In D109587#2998978, @kito-cheng wrote:

GNU toolchain is always emit arch attribute even user didn't write it in assembly file, and it will implied the info from the -march option or default arch setting, it's what you want to do in LLVM part IIUC.

But this implementation has some issue:

• Inline asm will emit .attribute 5, (.attribute Tag_RISCV_arch) again, that result the output will reject by the GNU assembler since .attribute Tag_RISCV_arch required to be appear before any instruction.
• NO new testcase added to demonstrate what you want to do, I would suggest you should add one, assemble a .s file to .o and then dump the attribute to make sure .attribute Tag_RISCV_arch has emitted properly.

So I think the goal of this patch is right way, but the implementation need some tweaks, especially the code gen change for inline asm.

Yes, that's what I want to do.
Which place or file is appropriate to add such testcase?

[zixuan-wu]

Add testcase

[zixuan-wu]

ping...

[jrtc27]

In D109587#3007651, @zixuan-wu wrote:

ping...

It's been less than 24h, this is unnecessary and rude

[zixuan-wu]

In D109587#3007655, @jrtc27 wrote:

In D109587#3007651, @zixuan-wu wrote:

ping...

It's been less than 24h, this is unnecessary and rude

Sorry to disturb. I have been working in a space plane whose speed is close to light. So, relatively, you guys are also close to light, which are seen slowly to give comments. :)

llvm/include/llvm/MC/MCParser/MCTargetAsmParser.h

public:
   virtual const MCExpr *applyModifierToExpr(const MCExpr *E,
                                             MCSymbolRefExpr::VariantKind,
                                             MCContext &Ctx) {
     return nullptr;
   }
 
   // For actions that have to be performed before a label is emitted
   virtual void doBeforeLabelEmit(MCSymbol *Symbol) {}
-  
   virtual void onLabelParsed(MCSymbol *Symbol) {}
 
   /// Ensure that all previously parsed instructions have been emitted to the
   /// output streamer, if the target does not emit them immediately.
   virtual void flushPendingInstructions(MCStreamer &Out) {}
 
   virtual const MCExpr *createTargetUnaryExpr(const MCExpr *E,
                                               AsmToken::TokenKind OperatorToken,
                                               MCContext &Ctx) {
     return nullptr;
   }
 
   // For any initialization at the beginning of parsing.
-  virtual void onBeginOfFile() {}
+  virtual void onBeginOfFile(bool NoFinalize) {}
 
   // For any checks or cleanups at the end of parsing.
-  virtual void onEndOfFile() {}
+  virtual void onEndOfFile(bool NoFinalize) {}
 };
 
 } // end namespace llvm
 
 #endif // LLVM_MC_MCPARSER_MCTARGETASMPARSER_H

llvm/lib/MC/MCParser/AsmParser.cpp

if (!Sec->getBeginSymbol()) {
       getStreamer().emitLabel(SectionStartSym);
       Sec->setBeginSymbol(SectionStartSym);
     }
     bool InsertResult = getContext().addGenDwarfSection(Sec);
     assert(InsertResult && ".text section should not have debug info yet");
     (void)InsertResult;
   }
 
-  getTargetParser().onBeginOfFile();
+  getTargetParser().onBeginOfFile(NoFinalize);
 
   // While we have input, parse each statement.
   while (Lexer.isNot(AsmToken::Eof)) {
     ParseStatementInfo Info(&AsmStrRewrites);
     bool Parsed = parseStatement(Info, nullptr);
 
     // If we have a Lexer Error we are on an Error Token. Load in Lexer Error
     // for printing ErrMsg via Lex() only if no (presumably better) parser error
     // exists.
     if (Parsed && !hasPendingError() && Lexer.getTok().is(AsmToken::Error)) {
       Lex();
     }
 
     // parseStatement returned true so may need to emit an error.
     printPendingErrors();
 
     // Skipping to the next line if needed.
     if (Parsed && !getLexer().isAtStartOfStatement())
       eatToEndOfStatement();
   }
 
-  getTargetParser().onEndOfFile();
+  getTargetParser().onEndOfFile(NoFinalize);
   printPendingErrors();
 
   // All errors should have been emitted.
   assert(!hasPendingError() && "unexpected error from parseStatement");
 
   getTargetParser().flushPendingInstructions(getStreamer());
 
   if (TheCondState.TheCond != StartingCondState.TheCond ||

llvm/lib/MC/MCParser/MasmParser.cpp

if (!Sec->getBeginSymbol()) {
       getStreamer().emitLabel(SectionStartSym);
       Sec->setBeginSymbol(SectionStartSym);
     }
     bool InsertResult = getContext().addGenDwarfSection(Sec);
     assert(InsertResult && ".text section should not have debug info yet");
     (void)InsertResult;
   }
 
-  getTargetParser().onBeginOfFile();
+  getTargetParser().onBeginOfFile(NoFinalize);
 
   // While we have input, parse each statement.
   while (Lexer.isNot(AsmToken::Eof) ||
          SrcMgr.getParentIncludeLoc(CurBuffer) != SMLoc()) {
     // Skip through the EOF at the end of an inclusion.
     if (Lexer.is(AsmToken::Eof))
       Lex();
 
while (Lexer.isNot(AsmToken::Eof) ||
     // parseStatement returned true so may need to emit an error.
     printPendingErrors();
 
     // Skipping to the next line if needed.
     if (Parsed && !getLexer().isAtStartOfStatement())
       eatToEndOfStatement();
   }
 
-  getTargetParser().onEndOfFile();
+  getTargetParser().onEndOfFile(NoFinalize);
   printPendingErrors();
 
   // All errors should have been emitted.
   assert(!hasPendingError() && "unexpected error from parseStatement");
 
   getTargetParser().flushPendingInstructions(getStreamer());
 
   if (TheCondState.TheCond != StartingCondState.TheCond ||

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

private:
   bool parseExpr(OperandVector &Operands);
   StringRef getTokenStr() const;
   AsmToken peekToken();
   AsmToken getToken() const;
   SMLoc getLoc() const;
   void lex();
 
 public:
-  void onBeginOfFile() override;
+  void onBeginOfFile(bool NoFinalize) override;
 
   OperandMatchResultTy parseOptionalOperand(OperandVector &Operands);
   OperandMatchResultTy parseOptionalOpr(OperandVector &Operands);
 
   OperandMatchResultTy parseExpTgt(OperandVector &Operands);
   OperandMatchResultTy parseSendMsgOp(OperandVector &Operands);
   OperandMatchResultTy parseInterpSlot(OperandVector &Operands);
   OperandMatchResultTy parseInterpAttr(OperandVector &Operands);
static const OptionalOperand AMDGPUOptionalOperandTable[] = {
   {"op_sel_hi", AMDGPUOperand::ImmTyOpSelHi, false, nullptr},
   {"neg_lo", AMDGPUOperand::ImmTyNegLo, false, nullptr},
   {"neg_hi", AMDGPUOperand::ImmTyNegHi, false, nullptr},
   {"blgp", AMDGPUOperand::ImmTyBLGP, false, nullptr},
   {"cbsz", AMDGPUOperand::ImmTyCBSZ, false, nullptr},
   {"abid", AMDGPUOperand::ImmTyABID, false, nullptr}
 };
 
-void AMDGPUAsmParser::onBeginOfFile() {
+void AMDGPUAsmParser::onBeginOfFile(bool NoFinalize) {
   if (!getParser().getStreamer().getTargetStreamer() ||
       getSTI().getTargetTriple().getArch() == Triple::r600)
     return;
 
   if (!getTargetStreamer().getTargetID())
     getTargetStreamer().initializeTargetID(getSTI(), getSTI().getFeatureString());
 
   if (isHsaAbiVersion3Or4(&getSTI()))

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

if (ABIName.endswith("f") &&
       errs() << "Hard-float 'd' ABI can't be used for a target that "
                 "doesn't support the D instruction set extension (ignoring "
                 "target-abi)\n";
     }
 
     const MCObjectFileInfo *MOFI = Parser.getContext().getObjectFileInfo();
     ParserOptions.IsPicEnabled = MOFI->isPositionIndependent();
   }
+
+  void onBeginOfFile(bool NoFinalize) override {
+    if (!NoFinalize)
+      getTargetStreamer().emitTargetAttributes(*STI);
+  }
 };
 
 /// RISCVOperand - Instances of this class represent a parsed machine
 /// instruction
 struct RISCVOperand : public MCParsedAsmOperand {
 
   enum class KindTy {
     Token,

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

void onEndOfFunction(SMLoc ErrorLoc) {
     getStreamer().emitLabel(TempSym);
     auto Start = MCSymbolRefExpr::create(LastFunctionLabel, getContext());
     auto End = MCSymbolRefExpr::create(TempSym, getContext());
     auto Expr =
         MCBinaryExpr::create(MCBinaryExpr::Sub, End, Start, getContext());
     getStreamer().emitELFSize(LastFunctionLabel, Expr);
   }
 
-  void onEndOfFile() override { ensureEmptyNestingStack(); }
+  void onEndOfFile(bool NoFinalize) override { ensureEmptyNestingStack(); }
 };
 } // end anonymous namespace
 
 // Force static initialization.
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeWebAssemblyAsmParser() {
   RegisterMCAsmParser<WebAssemblyAsmParser> X(getTheWebAssemblyTarget32());
   RegisterMCAsmParser<WebAssemblyAsmParser> Y(getTheWebAssemblyTarget64());
 }

llvm/test/MC/RISCV/attribute-with-insts-option.s

+## Test -mattr= option effects.
+## We do not provide '.attribute arch'.
+
+# RUN: llvm-mc -triple riscv32 -filetype=obj -mattr=+m -mattr=+a -mattr=+d -mattr=+c %s \
+# RUN:   | llvm-objdump --triple=riscv32 -d -M no-aliases - \
+# RUN:   | FileCheck -check-prefix=CHECK-INST %s
+
+# RUN: llvm-mc -triple riscv64 -filetype=obj -mattr=+m -mattr=+a -mattr=+d -mattr=+c %s \
+# RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
+# RUN:   | FileCheck -check-prefix=CHECK-INST %s
+
+# CHECK-INST: lr.w t0, (t1)
+lr.w t0, (t1)
+
+# CHECK-INST: c.addi a3, -32
+c.addi a3, -32
+
+# CHECK-INST: fmadd.d fa0, fa1, fa2, fa3, dyn
+fmadd.d f10, f11, f12, f13, dyn
+
+# CHECK-INST: fmadd.s fa0, fa1, fa2, fa3, dyn
+fmadd.s f10, f11, f12, f13, dyn
+
+# CHECK-INST: addi ra, sp, 2
+addi ra, sp, 2
+
+# CHECK-INST: mul a4, ra, s0
+mul a4, ra, s0

llvm/test/MC/RISCV/fde-reloc.s

 # RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+relax < %s \
 # RUN:     | llvm-readobj -r - | FileCheck %s
 # RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=-relax < %s \
 # RUN:     | llvm-readobj -r - | FileCheck %s
 
 # Ensure that the eh_frame records the symbolic difference with the paired
 # relocations always.
 
 func:
   .cfi_startproc
   ret
   .cfi_endproc
 
-# CHECK:   Section (4) .rela.eh_frame {
+# CHECK:   Section (5) .rela.eh_frame {
 # CHECK-NEXT:   0x1C R_RISCV_32_PCREL - 0x0
 # CHECK-NEXT:   0x20 R_RISCV_ADD32 - 0x0
 # CHECK-NEXT:   0x20 R_RISCV_SUB32 - 0x0
 # CHECK-NEXT: }

llvm/test/MC/RISCV/rvf-user-csr-names.s

 # RUN: llvm-mc %s -triple=riscv32 -riscv-no-aliases -mattr=+f -show-encoding \
 # RUN:     | FileCheck -check-prefixes=CHECK-INST,CHECK-ENC %s
 # RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+f < %s \
 # RUN:     | llvm-objdump -d --mattr=+f - \
 # RUN:     | FileCheck -check-prefix=CHECK-INST-ALIAS %s
-# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+f < %s \
+# RUN: llvm-mc -filetype=obj -triple riscv32  < %s \
 # RUN:     | llvm-objdump -d - \
 # RUN:     | FileCheck -check-prefix=CHECK-INST-ALIAS-NO-F %s
 #
 # RUN: llvm-mc %s -triple=riscv64 -riscv-no-aliases -mattr=+f -show-encoding \
 # RUN:     | FileCheck -check-prefixes=CHECK-INST,CHECK-ENC %s
 # RUN: llvm-mc -filetype=obj -triple riscv64 -mattr=+f < %s \
 # RUN:     | llvm-objdump -d --mattr=+f - \
 # RUN:     | FileCheck -check-prefix=CHECK-INST-ALIAS %s
-# RUN: llvm-mc -filetype=obj -triple riscv64 -mattr=+f < %s \
+# RUN: llvm-mc -filetype=obj -triple riscv64 < %s \
 # RUN:     | llvm-objdump -d - \
 # RUN:     | FileCheck -check-prefix=CHECK-INST-ALIAS-NO-F %s
 
 ##################################
 # User Floating Pont CSRs
 ##################################
 
 # fflags