[lld][RISCV] Merge subtarget features to get right attributes section

[eklepilkina]

Summary

As far as during lto bitcode fileas are used as linker input, the main way to get RISCV subtarget is taken extensions from target-features from functions attributes. Adding processing these attributes during creating TargetMachine in lto backend.

[jrtc27]

Target features in a function must not leak outside a function. They are per-function for a reason, and are distinct from those for the TargetMachine (but default to that). If you break that then you break IFUNCs.

[eklepilkina]

Target features in a function must not leak outside a function.

All target extensions are placed there and there is already such usage in https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp#L127. Where should I get extensions information in bitcode file from?

Unfortuantely, I can't find now the comment in code, but I exactly remember the comment that target features for linker in taken from bitcode functions attributes + if -mattr is provided is taken from there also.

[eklepilkina]

Could you please answer the question above?

[MaskRay]

I agree with @jrtc27 that this patch is fundamentally flawed. A function may get different attributes (usually extra ones) from the module due to Function Multi Versioning features (GNU function attributes target/target_version/target_clones).
These functions are dispatched at run-time. It is not correct to assume that the mere presence of a function with extra attributes changes the requirement of the module.

[eklepilkina]

It is not correct to assume that the mere presence of a function with extra attributes changes the requirement of the module.

So could you please clarify what should be used to provide target extensions to linker in case of bitcode files? I saw a comment that using function attributes is the preferable way and I provided the link to code that they are used in `const RISCVSubtarget *
RISCVTargetMachine::getSubtargetImpl`

I'm ready to make fix, butI want to understand what is the right way. May be use some other attribute connected with module or something like this?

lld/test/ELF/lto/riscv-incompatible-attributes.ll

+; REQUIRES: riscv
+
+; RUN: llvm-as %S/Inputs/riscv-attributes1.ll -o %t1.o
+; RUN: llvm-as %S/Inputs/riscv-attributes2.ll -o %t2.o
+; RUN: llvm-as %s -o %t3.o
+; RUN: not ld.lld %t1.o %t2.o %t3.o -o %t 2> %t.err
+; RUN: FileCheck %s --input-file %t.err
+
+; CHECK: standard extension 'zfinx' can't be specified with extension 'f'
+
+target datalayout = "e-m:e-p:32:32-i64:64-n32-S128"
+target triple = "riscv32-unknown-linux-gnu"
+attributes #0 = { "frame-pointer"="none" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+32bit,+i,+a,+m,+zfinx,-relax,-save-restore" }
+
+define void @main() #0 {
+  ret void
+}

lld/test/ELF/lto/riscv-merge-attributes.ll

 ; REQUIRES: riscv
 
 ; RUN: llvm-as %S/Inputs/riscv-attributes1.ll -o %t1.o
 ; RUN: llvm-as %S/Inputs/riscv-attributes2.ll -o %t2.o
 ; RUN: llvm-as %s -o %t3.o
 ; RUN: ld.lld %t1.o %t2.o %t3.o -o %t
 ; RUN: llvm-readelf --arch-specific %t | FileCheck %s
 
 ; CHECK:      BuildAttributes {
 ; CHECK-NEXT:   FormatVersion: 0x41
 ; CHECK-NEXT:   Section 1 {
-; CHECK-NEXT:     SectionLength: 27
+; CHECK-NEXT:     SectionLength: 59
 ; CHECK-NEXT:     Vendor: riscv
 ; CHECK-NEXT:     Tag: Tag_File (0x1)
-; CHECK-NEXT:     Size: 17
+; CHECK-NEXT:     Size: 49
 ; CHECK-NEXT:     FileAttributes {
 ; CHECK-NEXT:       Attribute {
 ; CHECK-NEXT:         Tag: 4
 ; CHECK-NEXT:         Value: 16
 ; CHECK-NEXT:         TagName: stack_align
 ; CHECK-NEXT:         Description: Stack alignment is 16-bytes
 ; CHECK-NEXT:       }
 ; CHECK-NEXT:       Attribute {
 ; CHECK-NEXT:         Tag: 5
 ; CHECK-NEXT:         TagName: arch
-; CHECK-NEXT:         Value: rv32i2p1
+; CHECK-NEXT:         Value: rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0_zbb1p0
 ; CHECK-NEXT:       }
 ; CHECK-NEXT:     }
 ; CHECK-NEXT:   }
 ; CHECK-NEXT: }
 
 target datalayout = "e-m:e-p:32:32-i64:64-n32-S128"
 target triple = "riscv32-unknown-linux-gnu"
 attributes #0 = { "frame-pointer"="none" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+32bit,+i,+a,+m,-relax,-save-restore" }
 
 define void @main() #0 {
   ret void
 }

llvm/lib/LTO/LTOBackend.cpp

if (!PassPlugin) {
              << "'. Request ignored.\n";
       continue;
     }
 
     PassPlugin->registerPassBuilderCallbacks(PB);
   }
 }
 
+static void mergeFnTargetFeatures(SubtargetFeatures &Features, Module &Mod) {
+  for (Function &F : Mod) {
+    Attribute FSAttr = F.getFnAttribute("target-features");
+    if (FSAttr.isValid())
+      Features.AddFeature(FSAttr.getValueAsString());
+  }
+}
+
 static std::unique_ptr<TargetMachine>
 createTargetMachine(const Config &Conf, const Target *TheTarget, Module &M) {
   StringRef TheTriple = M.getTargetTriple();
   SubtargetFeatures Features;
   Features.getDefaultSubtargetFeatures(Triple(TheTriple));
   for (const std::string &A : Conf.MAttrs)
     Features.AddFeature(A);
 
+  mergeFnTargetFeatures(Features, M);
+
   std::optional<Reloc::Model> RelocModel;
   if (Conf.RelocModel)
     RelocModel = *Conf.RelocModel;
   else if (M.getModuleFlag("PIC Level"))
     RelocModel =
         M.getPICLevel() == PICLevel::NotPIC ? Reloc::Static : Reloc::PIC_;
 
   std::optional<CodeModel::Model> CodeModel;

llvm/lib/Support/RISCVISAInfo.cpp

Error RISCVISAInfo::checkDependency() {
 
   if (HasZvl && !HasVector)
     return createStringError(
         errc::invalid_argument,
         "zvl*b requires v or zve* extension to also be specified");
 
   // Additional dependency checks.
   // TODO: The 'q' extension requires rv64.
-  // TODO: It is illegal to specify 'e' extensions with 'f' and 'd'.
+  if ((HasF || HasD) && HasE)
+    return createStringError(errc::invalid_argument,
+                             "standard user-level extension 'e' can't be "
+                             "specified with extensions 'f' and 'd'");
+
+  if (HasF && HasZfinx)
+    return createStringError(
+        errc::invalid_argument,
+        "standard extension 'zfinx' can't be specified with extension 'f'");
 
   return Error::success();
 }
 
 static const char *ImpliedExtsV[] = {"zvl128b", "zve64d", "f", "d"};
 static const char *ImpliedExtsZfhmin[] = {"f"};
 static const char *ImpliedExtsZfh[] = {"f"};
 static const char *ImpliedExtsZdinx[] = {"zfinx"};

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

if (ABIName.endswith("f") &&
                !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);
+    auto ABIInfo = RISCVABI::computeTargetABI(STI.getTargetTriple(),
+                                              STI.getFeatureBits(), ABIName);
+    if (!ABIInfo)
+      report_fatal_error(ABIInfo.takeError());
 
     const MCObjectFileInfo *MOFI = Parser.getContext().getObjectFileInfo();
     ParserOptions.IsPicEnabled = MOFI->isPositionIndependent();
   }
 };
 
 /// RISCVOperand - Instances of this class represent a parsed machine
 /// instruction

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

enum ABI {
   ABI_LP64,
   ABI_LP64F,
   ABI_LP64D,
   ABI_Unknown
 };
 
 // Returns the target ABI, or else a StringError if the requested ABIName is
 // not supported for the given TT and FeatureBits combination.
-ABI computeTargetABI(const Triple &TT, FeatureBitset FeatureBits,
+Expected<ABI> computeTargetABI(const Triple &TT, FeatureBitset FeatureBits,
                                StringRef ABIName);
 
 ABI getTargetABI(StringRef ABIName);
 
 // Returns the register used to hold the stack pointer after realignment.
 MCRegister getBPReg();
 
 // Returns the register holding shadow call stack pointer.
 MCRegister getSCSPReg();

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

 } // namespace RISCVSysReg
 
 namespace RISCVInsnOpcode {
 #define GET_RISCVOpcodesList_IMPL
 #include "RISCVGenSearchableTables.inc"
 } // namespace RISCVInsnOpcode
 
 namespace RISCVABI {
-ABI computeTargetABI(const Triple &TT, FeatureBitset FeatureBits,
+Expected<ABI> computeTargetABI(const Triple &TT, FeatureBitset FeatureBits,
                                StringRef ABIName) {
   auto TargetABI = getTargetABI(ABIName);
   bool IsRV64 = TT.isArch64Bit();
   bool IsRV32E = FeatureBits[RISCV::FeatureRV32E];
 
   if (!ABIName.empty() && TargetABI == ABI_Unknown) {
     errs()
         << "'" << ABIName
         << "' is not a recognized ABI for this target (ignoring target-abi)\n";
Expected<ABI> computeTargetABI(const Triple &TT, FeatureBitset FeatureBits,
   }
 
   if (TargetABI != ABI_Unknown)
     return TargetABI;
 
   // If no explicit ABI is given, try to compute the default ABI.
   auto ISAInfo = RISCVFeatures::parseFeatureBits(IsRV64, FeatureBits);
   if (!ISAInfo)
-    report_fatal_error(ISAInfo.takeError());
+    return std::move(ISAInfo.takeError());
   return getTargetABI((*ISAInfo)->computeDefaultABI());
 }
 
 ABI getTargetABI(StringRef ABIName) {
   auto TargetABI = StringSwitch<ABI>(ABIName)
                        .Case("ilp32", ABI_ILP32)
                        .Case("ilp32f", ABI_ILP32F)
                        .Case("ilp32d", ABI_ILP32D)

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

 
 // This part is for ELF object output.
 RISCVTargetELFStreamer::RISCVTargetELFStreamer(MCStreamer &S,
                                                const MCSubtargetInfo &STI)
     : RISCVTargetStreamer(S), CurrentVendor("riscv"), STI(STI) {
   MCAssembler &MCA = getStreamer().getAssembler();
   const FeatureBitset &Features = STI.getFeatureBits();
   auto &MAB = static_cast<RISCVAsmBackend &>(MCA.getBackend());
-  setTargetABI(RISCVABI::computeTargetABI(STI.getTargetTriple(), Features,
-                                          MAB.getTargetOptions().getABIName()));
+  auto ABIInfo = RISCVABI::computeTargetABI(
+      STI.getTargetTriple(), Features, MAB.getTargetOptions().getABIName());
+  if (!ABIInfo) {
+    errs() << toString(ABIInfo.takeError());
+    exit(1);
+  }
+  setTargetABI(*ABIInfo);
 }
 
 RISCVELFStreamer &RISCVTargetELFStreamer::getStreamer() {
   return static_cast<RISCVELFStreamer &>(Streamer);
 }
 
 void RISCVTargetELFStreamer::emitDirectiveOptionPush() {}
 void RISCVTargetELFStreamer::emitDirectiveOptionPop() {}

llvm/lib/Target/RISCV/RISCVSubtarget.cpp

if (TuneCPU.empty())
     TuneCPU = CPU;
 
   ParseSubtargetFeatures(CPU, TuneCPU, FS);
   if (Is64Bit) {
     XLenVT = MVT::i64;
     XLen = 64;
   }
 
-  TargetABI = RISCVABI::computeTargetABI(TT, getFeatureBits(), ABIName);
+  auto ABIInfo = RISCVABI::computeTargetABI(TT, getFeatureBits(), ABIName);
+  if (!ABIInfo)
+    report_fatal_error(ABIInfo.takeError());
+  TargetABI = *ABIInfo;
   RISCVFeatures::validate(TT, getFeatureBits());
   return *this;
 }
 
 RISCVSubtarget::RISCVSubtarget(const Triple &TT, StringRef CPU,
                                StringRef TuneCPU, StringRef FS,
                                StringRef ABIName, unsigned RVVVectorBitsMin,
                                unsigned RVVVectorBitsMax,

llvm/test/MC/RISCV/target-abi-invalid.s

 # RUN: llvm-mc -triple=riscv32 -target-abi lp64 < %s 2>&1 \
 # RUN:   | FileCheck -check-prefix=RV32I-LP64 %s
 # RUN: llvm-mc -triple=riscv32 -mattr=+f -target-abi lp64f < %s 2>&1 \
 # RUN:   | FileCheck -check-prefix=RV32IF-LP64F %s
 # RUN: llvm-mc -triple=riscv32 -mattr=+d -target-abi lp64d < %s 2>&1 \
 # RUN:   | FileCheck -check-prefix=RV32IFD-LP64D %s
 # RUN: llvm-mc -triple=riscv32 -mattr=+e -target-abi lp64 < %s 2>&1 \
 # RUN:   | FileCheck -check-prefix=RV32E-LP64 %s
-# RUN: llvm-mc -triple=riscv32 -mattr=+e,+f -target-abi lp64f < %s 2>&1 \
-# RUN:   | FileCheck -check-prefix=RV32EF-LP64F %s
-# RUN: llvm-mc -triple=riscv32 -mattr=+e,+d -target-abi lp64f < %s 2>&1 \
-# RUN:   | FileCheck -check-prefix=RV32EFD-LP64D %s
 
 # RV32I-LP64: 64-bit ABIs are not supported for 32-bit targets (ignoring target-abi)
 # RV32IF-LP64F: 64-bit ABIs are not supported for 32-bit targets (ignoring target-abi)
 # RV32IFD-LP64D: 64-bit ABIs are not supported for 32-bit targets (ignoring target-abi)
 # RV32E-LP64: 64-bit ABIs are not supported for 32-bit targets (ignoring target-abi)
-# RV32EF-LP64F: 64-bit ABIs are not supported for 32-bit targets (ignoring target-abi)
-# RV32EFD-LP64D: 64-bit ABIs are not supported for 32-bit targets (ignoring target-abi)
 
 # RUN: llvm-mc -triple=riscv32 -target-abi ilp32f < %s 2>&1 \
 # RUN:   | FileCheck -check-prefix=RV32I-ILP32F %s
 # RUN: llvm-mc -triple=riscv64 -target-abi lp64f < %s 2>&1 \
 # RUN:   | FileCheck -check-prefix=RV64I-LP64F %s
 
 # RV32I-ILP32F: Hard-float 'f' ABI can't be used for a target that doesn't support the F instruction set extension (ignoring target-abi)
 # RV64I-LP64F: Hard-float 'f' ABI can't be used for a target that doesn't support the F instruction set extension (ignoring target-abi)
 
 # RUN: llvm-mc -triple=riscv32 -target-abi ilp32d < %s 2>&1 \
 # RUN:   | FileCheck -check-prefix=RV32I-ILP32D %s
 # RUN: llvm-mc -triple=riscv32 -mattr=+f -target-abi ilp32d < %s 2>&1 \
 # RUN:   | FileCheck -check-prefix=RV32IF-ILP32D %s
 # RUN: llvm-mc -triple=riscv64 -target-abi lp64d < %s 2>&1 \
 # RUN:   | FileCheck -check-prefix=RV64I-LP64D %s
 # RUN: llvm-mc -triple=riscv64 -mattr=+f -target-abi lp64d < %s 2>&1 \
 # RUN:   | FileCheck -check-prefix=RV64IF-LP64D %s
 
 # RV32I-ILP32D: Hard-float 'd' ABI can't be used for a target that doesn't support the D instruction set extension (ignoring target-abi)
 # RV32IF-ILP32D: Hard-float 'd' ABI can't be used for a target that doesn't support the D instruction set extension (ignoring target-abi)
 # RV64I-LP64D: Hard-float 'd' ABI can't be used for a target that doesn't support the D instruction set extension (ignoring target-abi)
 # RV64IF-LP64D: Hard-float 'd' ABI can't be used for a target that doesn't support the D instruction set extension (ignoring target-abi)
 
 # RUN: llvm-mc -triple=riscv32 -mattr=+e -target-abi ilp32 < %s 2>&1 \
-# RUN:   | FileCheck -check-prefix=RV32EF-ILP32F %s
-# RUN: llvm-mc -triple=riscv32 -mattr=+e,+f -target-abi ilp32f < %s 2>&1 \
-# RUN:   | FileCheck -check-prefix=RV32EF-ILP32F %s
-# RUN: llvm-mc -triple=riscv32 -mattr=+e,+d -target-abi ilp32f < %s 2>&1 \
-# RUN:   | FileCheck -check-prefix=RV32EFD-ILP32F %s
-# RUN: llvm-mc -triple=riscv32 -mattr=+e,+d -target-abi ilp32d < %s 2>&1 \
-# RUN:   | FileCheck -check-prefix=RV32EFD-ILP32D %s
+# RUN:   | FileCheck -check-prefix=RV32E-ILP32 %s
 
 # RV32E-ILP32: Only the ilp32e ABI is supported for RV32E (ignoring target-abi)
-# RV32EF-ILP32F: Only the ilp32e ABI is supported for RV32E (ignoring target-abi)
-# RV32EFD-ILP32F: Only the ilp32e ABI is supported for RV32E (ignoring target-abi)
-# RV32EFD-ILP32D: Only the ilp32e ABI is supported for RV32E (ignoring target-abi)
 
 nop