[PoC][RISCV] Encode arch information in a new module flag meatadata 'riscv-isa-features'.

[khchen]

Summary

riscv-isa-features records the march information for a module.
RISCVAsmPrinter reads the riscv-isa-features to generate corrent arch
attribute if -mattr is missing.

Currently RISC-V encodes arch information in function attribute
'target-features', so there are two issues we want to fix.

1. In IFUNC case (ex. https://godbolt.org/z/a1oTbacn9), the final arch

attribute information is ambiguous because one compilation unit have
different target-feature value in function attribute.
We could not just union of all target-features, because in IFUNC case,
the base arch string is excepted in elf arch attribute.

2. During the LTO, clang driver will not pass -march option to LTO code

generator, because users maybe specify the incorrect -march value if
some linked external libraries have been compiled with different arch
extensions.

The probably way is adding a new module flag metadata, riscv-isa-features,
to records -march info for one compilation unit, and each
riscv-isa-features get combined when linking.

In this PoC patch, I think the potential issue is illegal arch combination
will report an error in code generation stage, not in linking stage.
(For example, zfinx is conflict with f/d/q, and Zceb and Zcec are
conflict with d).
I'm not sure having a target dependence behaivor in bitcode linking does
make sense or not.

Thanks for @jrtc27, @MaskRay to address this issue in D102926 and
D102925, and @kito-cheng's reminder of exclusive extensions issue.

[jrtc27]

Why can't we just save target-features itself as a module flag instead of inventing yet another equivalent encoding? Especially since a long bitfield is brittle, you can't reorder or remove elements without breaking bitcode compatibility.

[khchen]

In D106347#2890123, @jrtc27 wrote:

Why can't we just save target-features itself as a module flag instead of inventing yet another equivalent encoding? Especially since a long bitfield is brittle, you can't reorder or remove elements without breaking bitcode compatibility.

I think the benefit of using bitfield is making link behavior simpler. I will try to investigate another link behavior to handle two different target-features strings especially have + or -.

[khchen]

store target-features string as module flag.

[jrtc27]

clang/lib/CodeGen/CodeGenModule.cpp

835

Why is this building a list? Just use a string so it's in the same format as the function attributes? We already have parsers for that. Yes, it means you do a small amount of redundant work but having a single format in the IR for representing the same thing in every place is better than having multiple different ways of representing the same thing.

843

Just like we call it target-abi not riscv-abi, this should just be called target-features.

llvm/lib/Support/RISCVISAInfo.cpp

132–142

Why do we need to do any filtering? Just emit the whole thing.

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

53–60

This is unrelated

[jrtc27]

llvm/test/CodeGen/RISCV/riscv-isa-features.ll

1

Use update_llc_test_checks for these.

2

It's not overriding. It's just that #0 doesn't include -f,-d so applying #0 atop the default target-features keeps the default f and d features.

3

CHECK and ISA-F-D are not consistent with the check prefixes used in other RISC-V tests.

29

Space before }

[jrtc27]

clang/test/CodeGen/RISCV/riscv-metadata-isa-features-empty-target-feature.cpp

9

No check that the module flags actually includes this?

9–10

If you're not going to change the representation to be a single string, at least use a FileCheck variable so you can assert that this list is the one referenced by the module attribute above.

clang/test/CodeGen/RISCV/riscv-metadata-isa-features.c

15–21

These need check lines to ensure that these are actually being used for the right attribute. Especially !{!""}, that could easily be something else that you happen to match.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

184

Don't delete this

187

This doesn't make sense. The module flag should have been parsed and applied to the subtarget long ago. And an empty feature string means no features, not that they're missing. The fact that you need to change this code here is a sign that code elsewhere is wrong.

[khchen]

clang/lib/CodeGen/CodeGenModule.cpp

835

Because I think maybe the merge behavior AppendUnique is the best way to merge two module with different value of metadata flag, and it requires a metadata node with operands list.
Personally, I did not like to to have a new merge behavior to merge two target-features string, but maybe there is another better merging behavior I never thought,

In addition, some target features are not related to extension information, for example, +relax, +save-restore. Should we really need to record those unnecessary information in module?

What do you think?

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

187

What's excepted elf attribute if llc with -mattr=+f,+d but the riscv-isa-features module flag is +a,+c+m?
I think it will be +f, +d, so we need to make sure -mattr is empty before use the module flag. Does it make sense?

The module flag should have been parsed and applied to the subtarget long ago.

Sorry, I didn't understand it. The module flag riscv-isa-features is not applied to subtarget, I only use it to generate the correct extension .attribute.

Maybe I misunderstood something?

clang/lib/CodeGen/CodeGenModule.cpp

 //===--- CodeGenModule.cpp - Emit LLVM Code from ASTs for a Module --------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 // This coordinates the per-module state used while generating code.

 #include "llvm/IR/Module.h"
 #include "llvm/IR/ProfileSummary.h"
 #include "llvm/ProfileData/InstrProfReader.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Support/TimeProfiler.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace clang;
 using namespace CodeGen;
 
 static llvm::cl::opt<bool> LimitedCoverage(
     "limited-coverage-experimental", llvm::cl::ZeroOrMore, llvm::cl::Hidden,
     llvm::cl::desc("Emit limited coverage mapping information (experimental)"),
     llvm::cl::init(false));
if (   Arch == llvm::Triple::arm
       || Arch == llvm::Triple::armeb
       || Arch == llvm::Triple::thumb
       || Arch == llvm::Triple::thumbeb) {
     // The minimum width of an enum in bytes
     uint64_t EnumWidth = Context.getLangOpts().ShortEnums ? 1 : 4;
     getModule().addModuleFlag(llvm::Module::Error, "min_enum_size", EnumWidth);
   }
 
-  if (Arch == llvm::Triple::riscv32 || Arch == llvm::Triple::riscv64) {
-    StringRef ABIStr = Target.getABI();
-    llvm::LLVMContext &Ctx = TheModule.getContext();
-    getModule().addModuleFlag(llvm::Module::Error, "target-abi",
-                              llvm::MDString::get(Ctx, ABIStr));
-  }
-
   if (CodeGenOpts.SanitizeCfiCrossDso) {
     // Indicate that we want cross-DSO control flow integrity checks.
     getModule().addModuleFlag(llvm::Module::Override, "Cross-DSO CFI", 1);
   }
 
   if (CodeGenOpts.WholeProgramVTables) {
     // Indicate whether VFE was enabled for this module, so that the
     // vcall_visibility metadata added under whole program vtables is handled

 }
 
 void CodeGenModule::EmitBackendOptionsMetadata(
     const CodeGenOptions CodeGenOpts) {
   switch (getTriple().getArch()) {
   default:
     break;
   case llvm::Triple::riscv32:
-  case llvm::Triple::riscv64:
+  case llvm::Triple::riscv64: {
     getModule().addModuleFlag(llvm::Module::Error, "SmallDataLimit",
                               CodeGenOpts.SmallDataLimit);
+    StringRef ABIStr = Target.getABI();
+    llvm::LLVMContext &Ctx = TheModule.getContext();
+    getModule().addModuleFlag(llvm::Module::Error, "target-abi",
+                              llvm::MDString::get(Ctx, ABIStr));
+    std::vector<std::string> Features(getTarget().getTargetOpts().Features);
+    llvm::RISCVISAInfo::filterISAStrings(Features);
+    std::vector<llvm::Metadata *> Ops;
+    if (Features.empty()) {
+      Ops.push_back(llvm::MDString::get(Ctx, ""));
+    } else {
+      for (auto &ISAFeature : Features) {
+        Ops.push_back(llvm::MDString::get(Ctx, ISAFeature));
+      }
+    }
+    getModule().addModuleFlag(llvm::Module::AppendUnique, "riscv-isa-features",
+                              llvm::MDNode::get(Ctx, Ops));
     break;
   }
   }
+}
 
 void CodeGenModule::UpdateCompletedType(const TagDecl *TD) {
   // Make sure that this type is translated.
   Types.UpdateCompletedType(TD);
 }
 
 void CodeGenModule::RefreshTypeCacheForClass(const CXXRecordDecl *RD) {
   // Make sure that this type is translated.

clang/test/CodeGen/RISCV/riscv-metadata-isa-features-empty-target-feature.cpp

+// RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-feature +d -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: attributes #0 = {{[{].*}} "target-features"="+64bit,+d,+f" {{.*[}]}}
+// CHECK: attributes #1 = {{[{].*}} "target-features"="+64bit,+d,+f" {{.*[}]}}
+// CHECK: attributes #2 = {{[{].*}} "target-features"="+64bit,+d,+f" {{.*[}]}}
+// CHECK: attributes #3 = {{[{].*}} nounwind {{.*[}]}}
+
+// We need to record extension target-feature in riscv-isa-features module flag metadata because there is some empty target-features attribute
+// CHECK: !{{[0-9]+}} = !{i32 6, !"riscv-isa-features", !{{[0-9]+}}}
+// CHECK: !{{[0-9]+}} = !{!"+d", !"+f"}
+
+struct A {
+  A() {}
+};
+
+void test() noexcept {
+  A a;
+}
+
+int main() {
+  test();
+  return 0;
+}

clang/test/CodeGen/RISCV/riscv-metadata-isa-features-ifunc.c

+// RUN: %clang_cc1 -triple riscv64 -target-abi lp64 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: attributes #0 = {{[{].*}} "target-features"="+64bit" {{.*[}]}}
+// CHECK: attributes #1 = {{[{].*}} "target-features"="+64bit,+m" {{.*[}]}}
+
+// Record extension target-feature in riscv-isa-features module flag metadata for IFUNC case.
+// CHECK: !{{[0-9]+}} = !{i32 6, !"riscv-isa-features", !{{[0-9]+}}}
+// CHECK: !{{[0-9]+}} = !{!""}
+
+static int __attribute__((target("m")))
+mul_m(int x, int y) {
+  return x * y;
+}
+
+static int mul_i(int x, int y) {
+  return x * y;
+}
+
+static int (*foo_resolver(unsigned long hwcap))(int, int) {
+  if (hwcap & (1 << ('M' - 'A')))
+    return mul_m;
+  else
+    return mul_i;
+}
+
+int __attribute__((ifunc("foo_resolver")))
+foo(int, int);

clang/test/CodeGen/RISCV/riscv-metadata-isa-features.c

+// RUN: %clang_cc1 -triple riscv32 -emit-llvm -o - %s | FileCheck -check-prefix=BASE-ISA %s
+// RUN: %clang_cc1 -triple riscv32 -target-feature +d -emit-llvm -o - %s | FileCheck -check-prefix=RV32D-ISA %s
+// RUN: %clang_cc1 -triple riscv32 -emit-llvm -target-feature +f -target-feature +d -o - %s | FileCheck -check-prefix=RV32FD-ISA %s
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - %s | FileCheck -check-prefix=BASE-ISA %s
+// RUN: %clang_cc1 -triple riscv64 -target-feature +d -target-abi lp64d -emit-llvm -o - %s | FileCheck -check-prefix=RV64D-ISA %s
+// RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-feature +d -emit-llvm -o - %s | FileCheck -check-prefix=RV64FD-ISA %s
+// RUN: %clang_cc1 -triple riscv32 -emit-llvm -target-feature +f -target-feature +d -target-feature +experimental-v -target-feature +experimental-zfh -o - %s | FileCheck -check-prefix=RV32FDVZFH-ISA %s
+
+// RUN: %clang_cc1 -triple riscv32 -emit-llvm -target-feature +f -target-feature +d -target-feature +relax -target-feature -save-restore -o - %s | FileCheck -check-prefix=RV32FD-ISA %s
+
+// Test expected ISA result when giving conflicted -target-feautre
+// RUN: %clang_cc1 -triple riscv32 -emit-llvm -target-feature +d -target-feature -d -o - %s | FileCheck -check-prefix=RV32-NEG-D %s
+// RUN: %clang_cc1 -triple riscv32 -emit-llvm -target-feature -d -target-feature +d -o - %s | FileCheck -check-prefix=RV32D-ISA %s
+
+// BASE-ISA: !{{[0-9]+}} = !{!""}
+// RV32D-ISA: !{{[0-9]+}} = !{!"+d"}
+// RV32FD-ISA: !{{[0-9]+}} = !{!"+d", !"+f"}
+// RV32FDVZFH-ISA: !{{[0-9]+}} = !{!"+d", !"+experimental-v", !"+experimental-zfh", !"+f"}
+// RV64D-ISA: !{{[0-9]+}} = !{!"+d"}
+// RV64FD-ISA: !{{[0-9]+}} = !{!"+d", !"+f"}
+// RV32-NEG-D: !{{[0-9]+}} = !{!"-d"}

llvm/include/llvm/Support/RISCVISAInfo.h

public:
   const OrderedExtensionMap &getExtensions() const { return Exts; };
 
   unsigned getXLen() const { return XLen; };
   unsigned getFLen() const { return FLen; };
 
   bool hasExtension(StringRef Ext) const;
   std::string toString() const;
 
+  static void filterISAStrings(std::vector<std::string> &Features);
   static bool isSupportedExtension(StringRef Ext, bool CheckExperimental);
   static bool isSupportedExtension(StringRef Ext, unsigned MajorVersion,
                                    unsigned MinorVersion,
                                    bool CheckExperimental);
 
 private:
   unsigned XLen;
   unsigned FLen;

llvm/lib/Support/RISCVISAInfo.cpp

for (auto const &SupportedExtensionInfo :
     if (!SupportedExtensionInfo.Experimental)
       continue;
 
     return SupportedExtensionInfo.Version;
   }
   return None;
 }
 
+void RISCVISAInfo::filterISAStrings(std::vector<std::string> &Features) {
+  erase_if(Features, [](std::string &Feature) {
+    StringRef ExtName(Feature);
+    assert(ExtName.size() > 1 && (ExtName[0] == '+' || ExtName[0] == '-'));
+    ExtName = ExtName.drop_front(1); // Drop '+'
+    stripExperimentalPrefix(ExtName);
+    if (filterSupportedExtensionInfosByName(ExtName).empty())
+      return true;
+    return false;
+  });
+}
+
 bool RISCVISAInfo::isSupportedExtension(StringRef Ext, bool CheckExperimental) {
   bool IsExperimental = false;
   if (CheckExperimental)
     IsExperimental = stripExperimentalPrefix(Ext);
 
   for (auto const &SupportedExtensionInfo :
        filterSupportedExtensionInfosByName(Ext)) {
     if (CheckExperimental)

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_RISCVTARGETSTREAMER_H
 #define LLVM_LIB_TARGET_RISCV_RISCVTARGETSTREAMER_H
 
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 namespace llvm {
 
 class formatted_raw_ostream;
 
 class RISCVTargetStreamer : public MCTargetStreamer {
 public:
   RISCVTargetStreamer(MCStreamer &S);
public:
   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 emitTargetAttributes(const RISCVISAInfo &ISAInfo);
 };
 
 // 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/MCTargetDesc/RISCVTargetStreamer.cpp

if (STI.hasFeature(RISCV::FeatureRV32E))
     emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_4);
   else
     emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
   llvm::RISCVISAInfo ISAInfo;
   unsigned XLen = STI.hasFeature(RISCV::Feature64Bit) ? 64 : 32;
   std::vector<std::string> FeatureVector;
   RISCVFeatures::toFeatureVector(FeatureVector, STI.getFeatureBits());
-
   ISAInfo.parse(XLen, FeatureVector);
+  emitTargetAttributes(ISAInfo);
+}
 
+void RISCVTargetStreamer::emitTargetAttributes(const RISCVISAInfo &ISAInfo) {
+  if (ISAInfo.hasExtension("e"))
+    emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_4);
+  else
+    emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
   emitTextAttribute(RISCVAttrs::ARCH, ISAInfo.toString());
 }
 
 // This part is for ascii assembly output
 RISCVTargetAsmStreamer::RISCVTargetAsmStreamer(MCStreamer &S,
                                                formatted_raw_ostream &OS)
     : RISCVTargetStreamer(S), OS(OS) {}
 

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

 #include "llvm/CodeGen/MachineConstantPool.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSymbol.h"
+#include "llvm/Support/RISCVISAInfo.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;
 
 #define DEBUG_TYPE "asm-printer"
 
 STATISTIC(RISCVNumInstrsCompressed,
           "Number of RISC-V Compressed instructions emitted");
bool RISCVAsmPrinter::runOnMachineFunction(MachineFunction &MF) {
   STI = &NewSTI;
 
   SetupMachineFunction(MF);
   emitFunctionBody();
   return false;
 }
 
 void RISCVAsmPrinter::emitStartOfAsmFile(Module &M) {
-  if (TM.getTargetTriple().isOSBinFormatELF())
-    emitAttributes();
+  if (TM.getTargetTriple().isOSBinFormatELF()) {
+    RISCVTargetStreamer &RTS =
+        static_cast<RISCVTargetStreamer &>(*OutStreamer->getTargetStreamer());
+    const auto *ISAFeatureNodes =
+        dyn_cast_or_null<MDNode>(M.getModuleFlag("riscv-isa-features"));
+    if (STI->getFeatureString().empty() && ISAFeatureNodes) {
+      std::vector<std::string> Features;
+      for (const MDOperand &Feature : ISAFeatureNodes->operands()) {
+        auto MAttr = cast<MDString>(Feature.get())->getString();
+        Features.push_back(MAttr.str());
+      }
+      unsigned XLEN =
+          TM.getTargetTriple().getArch() == llvm::Triple::riscv32 ? 32 : 64;
+      RISCVISAInfo ISAInfo;
+      ISAInfo.parse(XLEN, Features);
+      RTS.emitTargetAttributes(ISAInfo);
+    } else {
+      RTS.emitTargetAttributes(*STI);
+    }
+  }
 }
-
 void RISCVAsmPrinter::emitEndOfAsmFile(Module &M) {
   RISCVTargetStreamer &RTS =
       static_cast<RISCVTargetStreamer &>(*OutStreamer->getTargetStreamer());
 
   if (TM.getTargetTriple().isOSBinFormatELF())
     RTS.finishAttributeSection();
 }
 

llvm/test/CodeGen/RISCV/riscv-isa-features.ll

+; RUN: llc -o - < %s | FileCheck %s
+; -mattr option would overwrite target-feature and module flag riscv-isa-features
+; RUN: llc -o - -mattr=+f,+d < %s | FileCheck %s -check-prefix=ISA-F-D
+; RUN: llc --filetype=obj -o - < %s | llvm-readelf -A - \
+; RUN:   | FileCheck %s -check-prefix=OBJFILE
+
+; CHECK:   .attribute      5, "rv64i2p0_m2p0_a2p0_c2p0"
+; ISA-F-D: .attribute      5, "rv64i2p0_f2p0_d2p0"
+
+; OBJFILE: TagName: arch
+; OBJFILE: Value: rv64i2p0_m2p0_a2p0_c2p0
+
+target triple = "riscv64-unknown-linux-gnu"
+
+define float @foo0(i32 %a) nounwind #0 {
+; CHECK:   call    __floatsisf@plt
+; ISA-F-D: fcvt.s.w        ft0, a0
+  %conv = sitofp i32 %a to float
+  ret float %conv
+}
+
+define float @foo1(i32 %a) nounwind #1 {
+; CHECK:   fcvt.s.w        ft0, a0
+; ISA-F-D: fcvt.s.w        ft0, a0
+  %conv = sitofp i32 %a to float
+  ret float %conv
+}
+
+attributes #0 = { "target-features"="+64bit,+a,+c,+m"}
+attributes #1 = { "target-features"="+64bit,+a,+c,+d,+f,+m"}
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 6, !"riscv-isa-features", !1}
+!1 = !{!"+a", !"+c", !"+m"}