[RISCV] add subtargets initialized with target feature

[khchen]

Summary

According to https://reviews.llvm.org/D67409#1737444, previous enabling LTO patch uses a wrong approach.
The target features have been encoded in bitcode files on a per-function basis, so clang driver does not need to pass them to LTO code generator.
So I reference ARM backend to init subtarget if there is target features attribute.

[lenary]

Nice, if this is the correct way to do it, then it's looking good.

I see x86 and arm both inspect the "soft-float" attribute, which seems to come from the -msoft-float clang option. Should RISC-V be using that to switch off the d and f target features?

I'm going to add @efriedma to the reviewers, as he flagged up this was implemented in the wrong way before.

[khchen]

In D70116#1741987, @lenary wrote:

Nice, if this is the correct way to do it, then it's looking good.

I see x86 and arm both inspect the "soft-float" attribute, which seems to come from the -msoft-float clang option. Should RISC-V be using that to switch off the d and f target features?

I think in RISC-V using -mabi is a convention way for specific calling convention in gcc and clang, and more clear than -msoft-float.
but unfortunately compiling source code with -mabi=ilp32 will not generate correct value like "use-soft-float"="true" in bitcode,
so we can fix clang (?) and let backend inspect the "soft-float" attribute, or fix clang driver to pass -mabi to LTO code generator in RISC-V target.

I'm going to add @efriedma to the reviewers, as he flagged up this was implemented in the wrong way before.

[khchen]

I found RISCVABI::computeTargetABI will check ABI with target feature,
so when I apply this patch and enabling lto, the checking get failed and set ABI to ABI_Unknown.
It's not easy to fix it because target-features is per-function basis but ABI is not.

In addition, I think my testcase is so weird and it does not make sense there are different isa extension are used in the same compilation unit...
maybe before checking the invalid ABI, backend need to gather all isa-extension target-feature attribute from functions and decide which standard extension should used if user does not specific -march.
or assume in RISC-V "isa extension target feature" should be specific in codegen and the clang driver treat them as specific case so driver will only pass "isa extension target feature" into LTO code generator.

[kito-cheng]

In addition, I think my testcase is so weird and it does not make sense there are different isa extension are used in the same compilation unit...

It's not weird, there is real use case in glibc work with IFUNC, target attribute for AArch64 and x86 can result different target feature in same compilation unit, and I believe such feature will implement for RISC-V in future.

void foo(){
}

void bar() __attribute__((target("sse4")));
void bar(){
}

[khchen]

In D70116#1745601, @khchen wrote:

I found RISCVABI::computeTargetABI will check ABI with target feature,
so when I apply this patch and enabling lto, the checking get failed and set ABI to ABI_Unknown.
It's not easy to fix it because target-features is per-function basis but ABI is not.

It looks like assembler need to read the assembly directive like .attribute arch, rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0 to initial the target feature before abi checking (computeTargetABI).
so I will fix this part (generate and parse assembly directive) later.

In addition, I think my testcase is so weird and it does not make sense there are different isa extension are used in the same compilation unit...
maybe before checking the invalid ABI, backend need to gather all isa-extension target-feature attribute from functions and decide which standard extension should used if user does not specific -march.
or assume in RISC-V "isa extension target feature" should be specific in codegen and the clang driver treat them as specific case so driver will only pass "isa extension target feature" into LTO code generator.

[lenary]

So am I right in thinking that neither foo nor foo2 will trigger the ABI_Unknown issue, and the issue is only coming from hand-written assembly files?

Actually, you may need to specify -target-abi in the top of the subtarget-features-std-ext.ll file.

[khchen]

In D70116#1761492, @lenary wrote:

So am I right in thinking that neither foo nor foo2 will trigger the ABI_Unknown issue, and the issue is only coming from hand-written assembly files?

Actually, you may need to specify -target-abi in the top of the subtarget-features-std-ext.ll file.

Sorry I didn't explain clearly,
The problem is caused by when I try to enable LTO and it need to pass mabi to LTO code generator (mabi is not encoded in bitcode)
like we compile below case with llc example.c -mtriple=riscv32 -mabi=ilp32f.

define float @foo(i32 %a) nounwind #0 {
  %conv = sitofp i32 %a to float
  ret float %conv
}

define float @foo2(i32 %a) nounwind #1{
  %conv = sitofp i32 %a to float
  ret float %conv
}

attributes #0 = { "target-features"="+f"}
attributes #1 = { "target-features"="+f"}

and they will show two error messages Hard-float 'f' ABI can't be used for a target that doesn't support the F instruction set extension.
so there is no any issue in subtarget-features-std-ext.ll without -target-abi.

when I started to fix this error, I found backend shares the ABI parsing/checking function (computeTargetABI) in codegen and MC layer when instance RISCVAsmBackend (note: IR function is not available at this point)
ref. https://github.com/llvm/llvm-project/commit/fea4957177315f83746dca90cb4c9013eb465c46

so this patch is workable, but it's not useful for LTO, not useful enough to support per function feature in backend, and then I'm not sure it's worth to upstream this patch.

[lenary]

In D70116#1761541, @khchen wrote:

In D70116#1761492, @lenary wrote:

So am I right in thinking that neither foo nor foo2 will trigger the ABI_Unknown issue, and the issue is only coming from hand-written assembly files?

Actually, you may need to specify -target-abi in the top of the subtarget-features-std-ext.ll file.

Sorry I didn't explain clearly,
The problem is caused by when I try to enable LTO and it need to pass mabi to LTO code generator (mabi is not encoded in bitcode)
like we compile below case with llc example.c -mtriple=riscv32 -mabi=ilp32f.

define float @foo(i32 %a) nounwind #0 {
  %conv = sitofp i32 %a to float
  ret float %conv
}

define float @foo2(i32 %a) nounwind #1{
  %conv = sitofp i32 %a to float
  ret float %conv
}

attributes #0 = { "target-features"="+f"}
attributes #1 = { "target-features"="+f"}

and they will show two error messages Hard-float 'f' ABI can't be used for a target that doesn't support the F instruction set extension.
so there is no any issue in subtarget-features-std-ext.ll without -target-abi.

when I started to fix this error, I found backend shares the ABI parsing/checking function (computeTargetABI) in codegen and MC layer when instance RISCVAsmBackend (note: IR function is not available at this point)
ref. https://github.com/llvm/llvm-project/commit/fea4957177315f83746dca90cb4c9013eb465c46

so this patch is workable, but it's not useful for LTO, not useful enough to support per function feature in backend, and then I'm not sure it's worth to upstream this patch.

This patch seems to solve the issue of working out the correct Subtarget on a per-function basis? This would suggest it is useful.

Presumably the next steps are to work out how to pass -target-abi through to the LTO backend via attributes? I was trying to see how other backends did this, but I'm not sure. I think the right way may be to have a feature per ABI, like ARM does here: https://github.com/llvm/llvm-project/blob/2d739f98d8a53e38bf9faa88cdb6b0c2a363fb77/clang/lib/Driver/ToolChains/Arch/ARM.cpp#L313-L319

It might be worth sending an email to llvm-dev, asking for guidance.

[khchen]

In D70116#1761610, @lenary wrote:

In D70116#1761541, @khchen wrote:

In D70116#1761492, @lenary wrote:

So am I right in thinking that neither foo nor foo2 will trigger the ABI_Unknown issue, and the issue is only coming from hand-written assembly files?

Actually, you may need to specify -target-abi in the top of the subtarget-features-std-ext.ll file.

Sorry I didn't explain clearly,
The problem is caused by when I try to enable LTO and it need to pass mabi to LTO code generator (mabi is not encoded in bitcode)
like we compile below case with llc example.c -mtriple=riscv32 -mabi=ilp32f.

define float @foo(i32 %a) nounwind #0 {
  %conv = sitofp i32 %a to float
  ret float %conv
}

define float @foo2(i32 %a) nounwind #1{
  %conv = sitofp i32 %a to float
  ret float %conv
}

attributes #0 = { "target-features"="+f"}
attributes #1 = { "target-features"="+f"}

and they will show two error messages Hard-float 'f' ABI can't be used for a target that doesn't support the F instruction set extension.
so there is no any issue in subtarget-features-std-ext.ll without -target-abi.

when I started to fix this error, I found backend shares the ABI parsing/checking function (computeTargetABI) in codegen and MC layer when instance RISCVAsmBackend (note: IR function is not available at this point)
ref. https://github.com/llvm/llvm-project/commit/fea4957177315f83746dca90cb4c9013eb465c46

so this patch is workable, but it's not useful for LTO, not useful enough to support per function feature in backend, and then I'm not sure it's worth to upstream this patch.

This patch seems to solve the issue of working out the correct Subtarget on a per-function basis? This would suggest it is useful.

Yes. Okay, agree with you.

Presumably the next steps are to work out how to pass -target-abi through to the LTO backend via attributes? I was trying to see how other backends did this, but I'm not sure. I think the right way may be to have a feature per ABI, like ARM does here: https://github.com/llvm/llvm-project/blob/2d739f98d8a53e38bf9faa88cdb6b0c2a363fb77/clang/lib/Driver/ToolChains/Arch/ARM.cpp#L313-L319

It might be worth sending an email to llvm-dev, asking for guidance.

my original plan is assume there is only one ABI in one compilation unit, so just pass -target-abi to LTO backend via option, and fix the above issues I meet.
But if in the future the object file allow different functions has different ABIs in one compilation unit, it should be have per function ABI feature in backend.
I noted there is no one target looks soft-float-abi attribute to decide ABI, they almost uses soft-float.
so if rv32imafc/ilp32 combination (like arm's softfp) is illegal, I think using soft-float is enough and a good ideal.

[lenary]

We definitely want to support rv32imafc/ilp32 and combinations like that.

At some point we're going to have to reckon with the fact that the ABI is described globally in the resulting object file, so cannot really be modelled on a per-function basis.

I think you're right that the solution for the moment is to pass -target-abi down to LTO, but only for RISC-V targets. We can work out what else we need to do for LTO once we're at that point.

[khchen]

add expected failed case, it fixed in https://reviews.llvm.org/D70837.

[efriedma]

LGTM

llvm/test/CodeGen/RISCV/subtarget-features-std-ext.ll

7

Not really an issue with this patch specifically, but backends shouldn't be writing directly to stderr. There's a diagnose() API on LLVMContext.

[khchen]

llvm/test/CodeGen/RISCV/subtarget-features-std-ext.ll

7

got it, I will fix it, thanks.

llvm/lib/Target/RISCV/RISCVTargetMachine.h

 #include "RISCVSubtarget.h"
 #include "llvm/CodeGen/SelectionDAGTargetInfo.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/Target/TargetMachine.h"
 
 namespace llvm {
 class RISCVTargetMachine : public LLVMTargetMachine {
   std::unique_ptr<TargetLoweringObjectFile> TLOF;
-  RISCVSubtarget Subtarget;
+  mutable StringMap<std::unique_ptr<RISCVSubtarget>> SubtargetMap;
 
 public:
   RISCVTargetMachine(const Target &T, const Triple &TT, StringRef CPU,
                      StringRef FS, const TargetOptions &Options,
                      Optional<Reloc::Model> RM, Optional<CodeModel::Model> CM,
                      CodeGenOpt::Level OL, bool JIT);
 
-  const RISCVSubtarget *getSubtargetImpl(const Function &) const override {
-    return &Subtarget;
-  }
+  const RISCVSubtarget *getSubtargetImpl(const Function &F) const override;
+  // DO NOT IMPLEMENT: There is no such thing as a valid default subtarget,
+  // subtargets are per-function entities based on the target-specific
+  // attributes of each function.
+  const RISCVSubtarget *getSubtargetImpl() const = delete;
 
   TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
 
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
 
   TargetTransformInfo getTargetTransformInfo(const Function &F) override;
 };
 }
 
 #endif

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

RISCVTargetMachine::RISCVTargetMachine(const Target &T, const Triple &TT,
                                        StringRef CPU, StringRef FS,
                                        const TargetOptions &Options,
                                        Optional<Reloc::Model> RM,
                                        Optional<CodeModel::Model> CM,
                                        CodeGenOpt::Level OL, bool JIT)
     : LLVMTargetMachine(T, computeDataLayout(TT), TT, CPU, FS, Options,
                         getEffectiveRelocModel(TT, RM),
                         getEffectiveCodeModel(CM, CodeModel::Small), OL),
-      TLOF(std::make_unique<RISCVELFTargetObjectFile>()),
-      Subtarget(TT, CPU, FS, Options.MCOptions.getABIName(), *this) {
+      TLOF(std::make_unique<RISCVELFTargetObjectFile>()) {
   initAsmInfo();
 }
 
+const RISCVSubtarget *
+RISCVTargetMachine::getSubtargetImpl(const Function &F) const {
+  Attribute CPUAttr = F.getFnAttribute("target-cpu");
+  Attribute FSAttr = F.getFnAttribute("target-features");
+
+  std::string CPU = !CPUAttr.hasAttribute(Attribute::None)
+                        ? CPUAttr.getValueAsString().str()
+                        : TargetCPU;
+  std::string FS = !FSAttr.hasAttribute(Attribute::None)
+                       ? FSAttr.getValueAsString().str()
+                       : TargetFS;
+  std::string Key = CPU + FS;
+  auto &I = SubtargetMap[Key];
+  if (!I) {
+    // This needs to be done before we create a new subtarget since any
+    // creation will depend on the TM and the code generation flags on the
+    // function that reside in TargetOptions.
+    resetTargetOptions(F);
+    I = std::make_unique<RISCVSubtarget>(TargetTriple, CPU, FS,
+                                         Options.MCOptions.getABIName(), *this);
+  }
+  return I.get();
+}
+
 TargetTransformInfo
 RISCVTargetMachine::getTargetTransformInfo(const Function &F) {
   return TargetTransformInfo(RISCVTTIImpl(this, F));
 }
 
 namespace {
 class RISCVPassConfig : public TargetPassConfig {
 public:

llvm/test/CodeGen/RISCV/subtarget-features-std-ext.ll

+; RUN: llc -mtriple=riscv32 < %s | FileCheck %s
+
+define float @foo(i32 %a) nounwind #0 {
+; CHECK-LABEL: foo:
+; CHECK: fcvt.s.w  ft0, a0
+  %conv = sitofp i32 %a to float
+  ret float %conv
+}
+
+define float @foo2(i32 %a) nounwind {
+; CHECK-LABEL: foo2:
+; CHECK: call __floatsisf
+  %conv = sitofp i32 %a to float
+  ret float %conv
+}
+
+attributes #0 = { "target-features"="+f"}