[RISCV] Support ABI checking with per function target-features

[khchen]

Summary

This is subsequent patch of https://reviews.llvm.org/D70116.

This is one of patches for enabling LTO support in RISCV, in some time LTO code generator will handle below case with -mabi=ilp32f, it's similar to llc example.c -mtriple=riscv32 -mabi=ilp32f.

define float @foo(i32 %a) nounwind #0 {
...
}

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

1. at this moment we assume one compilation unit only have same abi.
2. The target-features is per function attribute, and the original checking is at

too early stage so backned can't get the function attribute at that times.

so I think maybe moving the checking to RISCVTargetLowering is a good idea.

3. if users don't specific -mattr, the default target-feature come from IR attribute.
4. fix https://reviews.llvm.org/D70116 failed case

[khchen]

This comment was removed by khchen.

[khchen]

update summary.

[khchen]

Hi @lenary,
This patch is necessary for enabling LTO
backend does this mismatch checking before getSubtargetImpl which reads the target-feature info from IR function attribute, and it will reset the target-abi if mismatching happen.
because I don't want to break this mismatch checking feature, so I move this checking later than getSubtargetImpl execution.

[lenary]

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

200

I'm not sure we should be using errs() here directly, should we perhaps be using report_fatal_error here?

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

59

I don't think we should be changing the ABI here if the user already chose one explicitly.

[asb]

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

200

I implemented the RISCVBaseInfo checking using report_fatal_error initially, but moved to complaining via errs() after feedback that this better matched what other backends do in this case.

[khchen]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

59

I follow the same behavior to changing the ABI. please see https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/RISCV/Utils/RISCVBaseInfo.cpp#L59-L68. If we want to change the original behavior maybe it should be another patch?

[lenary]

LGTM!

[RKSimon]

@khchen We have a load of EXPENSIVE_CHECKS RISCV MC + asm failures, a lot of which appear to be related to these changes to RISCVAsmParser please can you take a look?

First appeared:
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/21599/

Current:
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/21630/steps/test-check-all/logs/stdio

[khchen]

In D70837#1824682, @RKSimon wrote:

@khchen We have a load of EXPENSIVE_CHECKS RISCV MC + asm failures, a lot of which appear to be related to these changes to RISCVAsmParser please can you take a look?

First appeared:
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/21599/

Current:
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/21630/steps/test-check-all/logs/stdio

Sorry, this is my bug.
I revert it https://reviews.llvm.org/rGcef838e65f9a2aeecf5e19431077bc16b01a79fb
It passed LLVM_ENABLE_EXPENSIVE_CHECKS on linux so I didn't catch it, Sorry again.
The root cause is std::string::back shall not be called on empty strings.

[khchen]

I fixed bug here, but I'm not sure this small fixed should be another commit or not.
@lenary, do you have any suggestion?

thanks.

[lenary]

LGTM. Let's re-land this.

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

#undef GET_OPERAND_DIAGNOSTIC_TYPES
   RISCVAsmParser(const MCSubtargetInfo &STI, MCAsmParser &Parser,
                  const MCInstrInfo &MII, const MCTargetOptions &Options)
       : MCTargetAsmParser(Options, STI, MII) {
     Parser.addAliasForDirective(".half", ".2byte");
     Parser.addAliasForDirective(".hword", ".2byte");
     Parser.addAliasForDirective(".word", ".4byte");
     Parser.addAliasForDirective(".dword", ".8byte");
     setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
+
+    auto ABIName = StringRef(Options.ABIName);
+    if (ABIName.endswith("f") &&
+        !getSTI().getFeatureBits()[RISCV::FeatureStdExtF]) {
+      errs() << "Hard-float 'f' ABI can't be used for a target that "
+                "doesn't support the F instruction set extension (ignoring "
+                "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";
+    }
   }
 };
 
 /// RISCVOperand - Instances of this class represent a parsed machine
 /// instruction
 struct RISCVOperand : public MCParsedAsmOperand {
 
   enum class KindTy {

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

RISCVTargetLowering::RISCVTargetLowering(const TargetMachine &TM,
     : TargetLowering(TM), Subtarget(STI) {
 
   if (Subtarget.isRV32E())
     report_fatal_error("Codegen not yet implemented for RV32E");
 
   RISCVABI::ABI ABI = Subtarget.getTargetABI();
   assert(ABI != RISCVABI::ABI_Unknown && "Improperly initialised target ABI");
 
+  if ((ABI == RISCVABI::ABI_ILP32F || ABI == RISCVABI::ABI_LP64F) &&
+      !Subtarget.hasStdExtF()) {
+    errs() << "Hard-float 'f' ABI can't be used for a target that "
+                "doesn't support the F instruction set extension (ignoring "
+                          "target-abi)\n";
+    ABI = Subtarget.is64Bit() ? RISCVABI::ABI_LP64 : RISCVABI::ABI_ILP32;
+  } else if ((ABI == RISCVABI::ABI_ILP32D || ABI == RISCVABI::ABI_LP64D) &&
+             !Subtarget.hasStdExtD()) {
+    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";
+    ABI = Subtarget.is64Bit() ? RISCVABI::ABI_LP64 : RISCVABI::ABI_ILP32;
+  }
+
   switch (ABI) {
   default:
     report_fatal_error("Don't know how to lower this ABI");
   case RISCVABI::ABI_ILP32:
   case RISCVABI::ABI_ILP32F:
   case RISCVABI::ABI_ILP32D:
   case RISCVABI::ABI_LP64:
   case RISCVABI::ABI_LP64F:

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

ABI computeTargetABI(const Triple &TT, FeatureBitset FeatureBits,
   } else if (ABIName.startswith("ilp32") && IsRV64) {
     errs() << "32-bit ABIs are not supported for 64-bit targets (ignoring "
               "target-abi)\n";
     TargetABI = ABI_Unknown;
   } else if (ABIName.startswith("lp64") && !IsRV64) {
     errs() << "64-bit ABIs are not supported for 32-bit targets (ignoring "
               "target-abi)\n";
     TargetABI = ABI_Unknown;
-  } else if (ABIName.endswith("f") && !FeatureBits[RISCV::FeatureStdExtF]) {
-    errs() << "Hard-float 'f' ABI can't be used for a target that "
-              "doesn't support the F instruction set extension (ignoring "
-              "target-abi)\n";
-    TargetABI = ABI_Unknown;
-  } else if (ABIName.endswith("d") && !FeatureBits[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";
-    TargetABI = ABI_Unknown;
   } else if (IsRV32E && TargetABI != ABI_ILP32E && TargetABI != ABI_Unknown) {
+    // TODO: move this checking to RISCVTargetLowering and RISCVAsmParser
     errs()
         << "Only the ilp32e ABI is supported for RV32E (ignoring target-abi)\n";
     TargetABI = ABI_Unknown;
   }
 
   if (TargetABI != ABI_Unknown)
     return TargetABI;
 

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

 ; RUN: 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 -mattr=-f -target-abi ilp32f <%s 2>&1 \
+; RUN:   | FileCheck -check-prefix=RV32I-ILP32F-FAILED %s
+
+; RV32I-ILP32F-FAILED: Hard-float 'f' ABI can't be used for a target that doesn't support the F instruction set extension
 
-; RV32IF-ILP32F: Hard-float 'f' ABI can't be used for a target that doesn't support the F instruction set extension (ignoring target-abi)
 
 define float @foo(i32 %a) nounwind #0 {
-; RV32IF-ILP32: # %bb.0:
-; RV32IF-ILP32-NEXT: fcvt.s.w  ft0, a0
+; RV32IF-ILP32: fcvt.s.w  ft0, a0
+; RV32IF-ILP32-NEXT: fmv.x.w a0, ft0
+; RV32IF-ILP32F: fcvt.s.w fa0, a0
+; RV32IF-ILP32F-NEXT: ret
   %conv = sitofp i32 %a to float
   ret float %conv
 }
 
 attributes #0 = { "target-features"="+f"}