[RISCV] Remove RISCVFeatures::validate function

[StephenFan]

Summary

Feature validation is done by parseFeature function. This RISCVFeature::validate function is redundant.

[StephenFan]

[RISCV] Generate correct ELF EFlags when .ll file has target-abi attribute

In the past, when construct RISCVAsmBackend, MCTargetOptions.ABIName would be passed and stored in RISCVAsmBackend.
But MCTargetOptions.ABIName can only be specified by -target-abi xxx in command line, if the .ll file has target-abi attribute, the codegen module will ignore it. And the generated object file would have incorrect EFlags value.

https://github.com/llvm/llvm-project/issues/50591 also caused by this problem.

This patch override the AsmPrinter::emitFunctionEntryLabel function and use it to set the target abi value that get from .ll file's target-abi attribute. And storing the target-abi in RISCVTargetStreamer instead of RISCVAsmBackend.

[craig.topper]

In D121073#3366178, @StephenFan wrote:

[RISCV] Generate correct ELF EFlags when .ll file has target-abi attribute

In the past, when construct RISCVAsmBackend, MCTargetOptions.ABIName would be passed and stored in RISCVAsmBackend.
But MCTargetOptions.ABIName can only be specified by -target-abi xxx in command line, if the .ll file has target-abi attribute, the codegen module will ignore it. And the generated object file would have incorrect EFlags value.

https://github.com/llvm/llvm-project/issues/50591 also caused by this problem.

This patch override the AsmPrinter::emitFunctionEntryLabel function and use it to set the target abi value that get from .ll file's target-abi attribute. And storing the target-abi in RISCVTargetStreamer instead of RISCVAsmBackend.

Was this supposed to be a new review?

[StephenFan]

undo

[StephenFan]

ping.

[StephenFan]

Ping. :)

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

if (ABIName.endswith("f") &&
                 "target-abi)\n";
     } else if (ABIName.endswith("d") &&
                !getSTI().getFeatureBits()[RISCV::FeatureStdExtD]) {
       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";
     }
 
+    // Use computeTargetABI to check if ABIName is valid. If invalid, output
+    // error message.
+    RISCVABI::computeTargetABI(STI.getTargetTriple(), STI.getFeatureBits(),
+                               ABIName);
+
     const MCObjectFileInfo *MOFI = Parser.getContext().getObjectFileInfo();
     ParserOptions.IsPicEnabled = MOFI->isPositionIndependent();
   }
 };
 
 /// RISCVOperand - Instances of this class represent a parsed machine
 /// instruction
 struct RISCVOperand : public MCParsedAsmOperand {

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h

 class raw_ostream;
 
 class RISCVAsmBackend : public MCAsmBackend {
   const MCSubtargetInfo &STI;
   uint8_t OSABI;
   bool Is64Bit;
   bool ForceRelocs = false;
   const MCTargetOptions &TargetOptions;
-  RISCVABI::ABI TargetABI = RISCVABI::ABI_Unknown;
 
 public:
   RISCVAsmBackend(const MCSubtargetInfo &STI, uint8_t OSABI, bool Is64Bit,
                   const MCTargetOptions &Options)
       : MCAsmBackend(support::little), STI(STI), OSABI(OSABI), Is64Bit(Is64Bit),
-        TargetOptions(Options) {
-    TargetABI = RISCVABI::computeTargetABI(
-        STI.getTargetTriple(), STI.getFeatureBits(), Options.getABIName());
-    RISCVFeatures::validate(STI.getTargetTriple(), STI.getFeatureBits());
-  }
+        TargetOptions(Options) {}
   ~RISCVAsmBackend() override = default;
 
   void setForceRelocs() { ForceRelocs = true; }
 
   // Return Size with extra Nop Bytes for alignment directive in code section.
   bool shouldInsertExtraNopBytesForCodeAlign(const MCAlignFragment &AF,
                                              unsigned &Size) override;
 
bool relaxDwarfLineAddr(MCDwarfLineAddrFragment &DF, MCAsmLayout &Layout,
                           bool &WasRelaxed) const override;
   bool relaxDwarfCFA(MCDwarfCallFrameFragment &DF, MCAsmLayout &Layout,
                      bool &WasRelaxed) const override;
 
   bool writeNopData(raw_ostream &OS, uint64_t Count,
                     const MCSubtargetInfo *STI) const override;
 
   const MCTargetOptions &getTargetOptions() const { return TargetOptions; }
-  RISCVABI::ABI getTargetABI() const { return TargetABI; }
 };
 }
 
 #endif

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

 
 // Returns the register holding shadow call stack pointer.
 MCRegister getSCSPReg();
 
 } // namespace RISCVABI
 
 namespace RISCVFeatures {
 
-// Validates if the given combination of features are valid for the target
-// triple. Exits with report_fatal_error if not.
-void validate(const Triple &TT, const FeatureBitset &FeatureBits);
-
 llvm::Expected<std::unique_ptr<RISCVISAInfo>>
 parseFeatureBits(bool IsRV64, const FeatureBitset &FeatureBits);
 
 } // namespace RISCVFeatures
 
 namespace RISCVVType {
 // Is this a SEW value that can be encoded into the VTYPE format.
 inline static bool isValidSEW(unsigned SEW) {

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp

 
 // Returns the register holding shadow call stack pointer.
 MCRegister getSCSPReg() { return RISCV::X18; }
 
 } // namespace RISCVABI
 
 namespace RISCVFeatures {
 
-void validate(const Triple &TT, const FeatureBitset &FeatureBits) {
-  if (TT.isArch64Bit() && !FeatureBits[RISCV::Feature64Bit])
-    report_fatal_error("RV64 target requires an RV64 CPU");
-  if (!TT.isArch64Bit() && FeatureBits[RISCV::Feature64Bit])
-    report_fatal_error("RV32 target requires an RV32 CPU");
-  if (TT.isArch64Bit() && FeatureBits[RISCV::FeatureRV32E])
-    report_fatal_error("RV32E can't be enabled for an RV64 target");
-}
-
 llvm::Expected<std::unique_ptr<RISCVISAInfo>>
 parseFeatureBits(bool IsRV64, const FeatureBitset &FeatureBits) {
   unsigned XLen = IsRV64 ? 64 : 32;
   std::vector<std::string> FeatureVector;
   // Convert FeatureBitset to FeatureVector.
   for (auto Feature : RISCVFeatureKV) {
     if (FeatureBits[Feature.Value] &&
         llvm::RISCVISAInfo::isSupportedExtensionFeature(Feature.Key))

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h

struct AttributeItem {
     unsigned IntValue;
     std::string StringValue;
   };
 
   StringRef CurrentVendor;
   SmallVector<AttributeItem, 64> Contents;
 
   MCSection *AttributeSection = nullptr;
+  const MCSubtargetInfo &STI;
 
   AttributeItem *getAttributeItem(unsigned Attribute) {
     for (size_t i = 0; i < Contents.size(); ++i)
       if (Contents[i].Tag == Attribute)
         return &Contents[i];
     return nullptr;
   }
 
public:
   void emitDirectiveOptionPush() override;
   void emitDirectiveOptionPop() override;
   void emitDirectiveOptionPIC() override;
   void emitDirectiveOptionNoPIC() override;
   void emitDirectiveOptionRVC() override;
   void emitDirectiveOptionNoRVC() override;
   void emitDirectiveOptionRelax() override;
   void emitDirectiveOptionNoRelax() override;
+
+  void finish() override;
 };
 
 MCELFStreamer *createRISCVELFStreamer(MCContext &C,
                                       std::unique_ptr<MCAsmBackend> MAB,
                                       std::unique_ptr<MCObjectWriter> MOW,
                                       std::unique_ptr<MCCodeEmitter> MCE,
                                       bool RelaxAll);
 }
 #endif

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

 #include "llvm/MC/MCAsmBackend.h"
 #include "llvm/MC/MCAssembler.h"
 #include "llvm/MC/MCCodeEmitter.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCSectionELF.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCValue.h"
-#include "llvm/Support/Casting.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/RISCVAttributes.h"
 
 using namespace llvm;
 
 // This part is for ELF object output.
 RISCVTargetELFStreamer::RISCVTargetELFStreamer(MCStreamer &S,
                                                const MCSubtargetInfo &STI)
-    : RISCVTargetStreamer(S), CurrentVendor("riscv") {
+    : RISCVTargetStreamer(S), CurrentVendor("riscv"), STI(STI) {
   MCAssembler &MCA = getStreamer().getAssembler();
   const FeatureBitset &Features = STI.getFeatureBits();
   auto &MAB = static_cast<RISCVAsmBackend &>(MCA.getBackend());
-  RISCVABI::ABI ABI = MAB.getTargetABI();
-  assert(ABI != RISCVABI::ABI_Unknown && "Improperly initialised target ABI");
-
-  unsigned EFlags = MCA.getELFHeaderEFlags();
-
-  if (Features[RISCV::FeatureStdExtC])
-    EFlags |= ELF::EF_RISCV_RVC;
-
-  switch (ABI) {
-  case RISCVABI::ABI_ILP32:
-  case RISCVABI::ABI_LP64:
-    break;
-  case RISCVABI::ABI_ILP32F:
-  case RISCVABI::ABI_LP64F:
-    EFlags |= ELF::EF_RISCV_FLOAT_ABI_SINGLE;
-    break;
-  case RISCVABI::ABI_ILP32D:
-  case RISCVABI::ABI_LP64D:
-    EFlags |= ELF::EF_RISCV_FLOAT_ABI_DOUBLE;
-    break;
-  case RISCVABI::ABI_ILP32E:
-    EFlags |= ELF::EF_RISCV_RVE;
-    break;
-  case RISCVABI::ABI_Unknown:
-    llvm_unreachable("Improperly initialised target ABI");
-  }
-
-  MCA.setELFHeaderEFlags(EFlags);
+  setTargetABI(RISCVABI::computeTargetABI(STI.getTargetTriple(), Features,
+                                          MAB.getTargetOptions().getABIName()));
 }
 
 MCELFStreamer &RISCVTargetELFStreamer::getStreamer() {
   return static_cast<MCELFStreamer &>(Streamer);
 }
 
 void RISCVTargetELFStreamer::emitDirectiveOptionPush() {}
 void RISCVTargetELFStreamer::emitDirectiveOptionPop() {}
case AttributeType::NumericAndText:
       Result += getULEB128Size(item.IntValue);
       Result += item.StringValue.size() + 1; // string + '\0';
       break;
     }
   }
   return Result;
 }
 
+void RISCVTargetELFStreamer::finish() {
+  RISCVTargetStreamer::finish();
+  MCAssembler &MCA = getStreamer().getAssembler();
+  const FeatureBitset &Features = STI.getFeatureBits();
+  RISCVABI::ABI ABI = getTargetABI();
+  assert(ABI != RISCVABI::ABI_Unknown && "Improperly initialized target ABI");
+
+  unsigned EFlags = MCA.getELFHeaderEFlags();
+
+  if (Features[RISCV::FeatureStdExtC])
+    EFlags |= ELF::EF_RISCV_RVC;
+
+  switch (ABI) {
+  case RISCVABI::ABI_ILP32:
+  case RISCVABI::ABI_LP64:
+    break;
+  case RISCVABI::ABI_ILP32F:
+  case RISCVABI::ABI_LP64F:
+    EFlags |= ELF::EF_RISCV_FLOAT_ABI_SINGLE;
+    break;
+  case RISCVABI::ABI_ILP32D:
+  case RISCVABI::ABI_LP64D:
+    EFlags |= ELF::EF_RISCV_FLOAT_ABI_DOUBLE;
+    break;
+  case RISCVABI::ABI_ILP32E:
+    EFlags |= ELF::EF_RISCV_RVE;
+    break;
+  case RISCVABI::ABI_Unknown:
+    llvm_unreachable("Improperly initialised target ABI");
+  }
+
+  MCA.setELFHeaderEFlags(EFlags);
+}
+
 namespace {
 class RISCVELFStreamer : public MCELFStreamer {
   static std::pair<unsigned, unsigned> getRelocPairForSize(unsigned Size) {
     switch (Size) {
     default:
       llvm_unreachable("unsupported fixup size");
     case 1:
       return std::make_pair(RISCV::fixup_riscv_add_8, RISCV::fixup_riscv_sub_8);

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp

 
 static MCSubtargetInfo *createRISCVMCSubtargetInfo(const Triple &TT,
                                                    StringRef CPU, StringRef FS) {
   if (CPU.empty())
     CPU = TT.isArch64Bit() ? "generic-rv64" : "generic-rv32";
   if (CPU == "generic")
     report_fatal_error(Twine("CPU 'generic' is not supported. Use ") +
                        (TT.isArch64Bit() ? "generic-rv64" : "generic-rv32"));
-  return createRISCVMCSubtargetInfoImpl(TT, CPU, /*TuneCPU*/ CPU, FS);
+
+  MCSubtargetInfo *STI =
+      createRISCVMCSubtargetInfoImpl(TT, CPU, /*TuneCPU*/ CPU, FS);
+
+  // Check if Feature string is valid
+  auto ISAInfo =
+      RISCVFeatures::parseFeatureBits(TT.isArch64Bit(), STI->getFeatureBits());
+  if (!ISAInfo)
+    report_fatal_error(ISAInfo.takeError());
+  else
+    return STI;
 }
 
 static MCInstPrinter *createRISCVMCInstPrinter(const Triple &T,
                                                unsigned SyntaxVariant,
                                                const MCAsmInfo &MAI,
                                                const MCInstrInfo &MII,
                                                const MCRegisterInfo &MRI) {
   return new RISCVInstPrinter(MAI, MII, MRI);

llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h

 //===-- RISCVTargetStreamer.h - RISCV Target Streamer ----------*- C++ -*--===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_RISCV_MCTARGETDESC_RISCVTARGETSTREAMER_H
 #define LLVM_LIB_TARGET_RISCV_MCTARGETDESC_RISCVTARGETSTREAMER_H
 
+#include "RISCV.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 
 namespace llvm {
 
 class formatted_raw_ostream;
 
 class RISCVTargetStreamer : public MCTargetStreamer {
+  RISCVABI::ABI TargetABI = RISCVABI::ABI_Unknown;
+
 public:
   RISCVTargetStreamer(MCStreamer &S);
   void finish() override;
 
   virtual void emitDirectiveOptionPush();
   virtual void emitDirectiveOptionPop();
   virtual void emitDirectiveOptionPIC();
   virtual void emitDirectiveOptionNoPIC();
   virtual void emitDirectiveOptionRVC();
   virtual void emitDirectiveOptionNoRVC();
   virtual void emitDirectiveOptionRelax();
   virtual void emitDirectiveOptionNoRelax();
   virtual void emitAttribute(unsigned Attribute, unsigned Value);
   virtual void finishAttributeSection();
   virtual void emitTextAttribute(unsigned Attribute, StringRef String);
   virtual void emitIntTextAttribute(unsigned Attribute, unsigned IntValue,
                                     StringRef StringValue);
 
   void emitTargetAttributes(const MCSubtargetInfo &STI);
+  void setTargetABI(RISCVABI::ABI ABI) { TargetABI = ABI; }
+  RISCVABI::ABI getTargetABI() const { return TargetABI; }
 };
 
 // This part is for ascii assembly output
 class RISCVTargetAsmStreamer : public RISCVTargetStreamer {
   formatted_raw_ostream &OS;
 
   void finishAttributeSection() override;
   void emitAttribute(unsigned Attribute, unsigned Value) override;

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

 
 #define DEBUG_TYPE "asm-printer"
 
 STATISTIC(RISCVNumInstrsCompressed,
           "Number of RISC-V Compressed instructions emitted");
 
 namespace {
 class RISCVAsmPrinter : public AsmPrinter {
-  const MCSubtargetInfo *STI;
+  const MCSubtargetInfo *MCSTI;
+  const RISCVSubtarget *STI;
+  RISCVABI::ABI ABI = RISCVABI::ABI_Unknown;
 
 public:
   explicit RISCVAsmPrinter(TargetMachine &TM,
                            std::unique_ptr<MCStreamer> Streamer)
-      : AsmPrinter(TM, std::move(Streamer)), STI(TM.getMCSubtargetInfo()) {}
+      : AsmPrinter(TM, std::move(Streamer)), MCSTI(TM.getMCSubtargetInfo()) {}
 
   StringRef getPassName() const override { return "RISCV Assembly Printer"; }
 
   bool runOnMachineFunction(MachineFunction &MF) override;
 
   void emitInstruction(const MachineInstr *MI) override;
 
   bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
                        const char *ExtraCode, raw_ostream &OS) override;
   bool PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
                              const char *ExtraCode, raw_ostream &OS) override;
 
   void EmitToStreamer(MCStreamer &S, const MCInst &Inst);
   bool emitPseudoExpansionLowering(MCStreamer &OutStreamer,
                                    const MachineInstr *MI);
 
   // Wrapper needed for tblgenned pseudo lowering.
   bool lowerOperand(const MachineOperand &MO, MCOperand &MCOp) const {
     return LowerRISCVMachineOperandToMCOperand(MO, MCOp, *this);
   }
 
   void emitStartOfAsmFile(Module &M) override;
   void emitEndOfAsmFile(Module &M) override;
 
+  void emitFunctionEntryLabel() override;
+
 private:
   void emitAttributes();
 };
 }
 
 #define GEN_COMPRESS_INSTR
 #include "RISCVGenCompressInstEmitter.inc"
 void RISCVAsmPrinter::EmitToStreamer(MCStreamer &S, const MCInst &Inst) {

 }
 
 bool RISCVAsmPrinter::runOnMachineFunction(MachineFunction &MF) {
   // Set the current MCSubtargetInfo to a copy which has the correct
   // feature bits for the current MachineFunction
   MCSubtargetInfo &NewSTI =
     OutStreamer->getContext().getSubtargetCopy(*TM.getMCSubtargetInfo());
   NewSTI.setFeatureBits(MF.getSubtarget().getFeatureBits());
-  STI = &NewSTI;
+  MCSTI = &NewSTI;
+  STI = &MF.getSubtarget<RISCVSubtarget>();
 
   SetupMachineFunction(MF);
   emitFunctionBody();
   return false;
 }
 
 void RISCVAsmPrinter::emitStartOfAsmFile(Module &M) {
   if (TM.getTargetTriple().isOSBinFormatELF())
     emitAttributes();
 }
 
 void RISCVAsmPrinter::emitEndOfAsmFile(Module &M) {
   RISCVTargetStreamer &RTS =
       static_cast<RISCVTargetStreamer &>(*OutStreamer->getTargetStreamer());
 
   if (TM.getTargetTriple().isOSBinFormatELF())
     RTS.finishAttributeSection();
 }
 
 void RISCVAsmPrinter::emitAttributes() {
   RISCVTargetStreamer &RTS =
       static_cast<RISCVTargetStreamer &>(*OutStreamer->getTargetStreamer());
-  RTS.emitTargetAttributes(*STI);
+  RTS.emitTargetAttributes(*MCSTI);
+}
+
+void RISCVAsmPrinter::emitFunctionEntryLabel() {
+  AsmPrinter::emitFunctionEntryLabel();
+  RISCVTargetStreamer &RTS =
+      static_cast<RISCVTargetStreamer &>(*OutStreamer->getTargetStreamer());
+  RTS.setTargetABI(STI->getTargetABI());
 }
 
 // Force static initialization.
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeRISCVAsmPrinter() {
   RegisterAsmPrinter<RISCVAsmPrinter> X(getTheRISCV32Target());
   RegisterAsmPrinter<RISCVAsmPrinter> Y(getTheRISCV64Target());
 }

llvm/lib/Target/RISCV/RISCVSubtarget.cpp

RISCVSubtarget::initializeSubtargetDependencies(const Triple &TT, StringRef CPU,
 
   ParseSubtargetFeatures(CPU, TuneCPU, FS);
   if (Is64Bit) {
     XLenVT = MVT::i64;
     XLen = 64;
   }
 
   TargetABI = RISCVABI::computeTargetABI(TT, getFeatureBits(), ABIName);
-  RISCVFeatures::validate(TT, getFeatureBits());
   return *this;
 }
 
 RISCVSubtarget::RISCVSubtarget(const Triple &TT, StringRef CPU,
                                StringRef TuneCPU, StringRef FS,
                                StringRef ABIName, const TargetMachine &TM)
     : RISCVGenSubtargetInfo(TT, CPU, TuneCPU, FS),
       UserReservedRegister(RISCV::NUM_TARGET_REGS),

llvm/test/CodeGen/RISCV/module-target-abi2.ll

 ; RUN: llc -mtriple=riscv32 < %s 2>&1 \
 ; RUN:   | FileCheck -check-prefix=DEFAULT %s
 ; RUN: not --crash llc -mtriple=riscv32 -target-abi ilp32 < %s 2>&1 \
 ; RUN:   | FileCheck -check-prefix=RV32IF-ILP32 %s
 ; RUN: llc -mtriple=riscv32 -target-abi ilp32f < %s 2>&1 \
 ; RUN:   | FileCheck -check-prefix=RV32IF-ILP32F %s
 ; RUN: llc -mtriple=riscv32 -filetype=obj < %s | llvm-readelf -h - | FileCheck -check-prefixes=FLAGS %s
 
 ; RV32IF-ILP32: -target-abi option != target-abi module flag
 
-; FLAGS: Flags: 0x0
-; // this should be "Flags :0x2, single-float ABI", it will be fixed later.
+; FLAGS: Flags: 0x2, single-float ABI
 
 define float @foo(i32 %a) nounwind #0 {
 ; DEFAULT: # %bb.0:
 ; DEFAULT-NEXT: fcvt.s.w fa0, a0
 ; DEFAULT-NEXT: ret
 ; RV32IF-ILP32F: # %bb.0:
 ; RV32IF-ILP32F: fcvt.s.w fa0, a0
 ; RV32IF-ILP32F: ret
   %conv = sitofp i32 %a to float
   ret float %conv
 }
 
 attributes #0 = { "target-features"="+f"}
 !llvm.module.flags = !{!0}
 !0 = !{i32 1, !"target-abi", !"ilp32f"}