[RISCV][NFC] Move ilp32e/rv32e checks to AsmParser and TargetLowering

[asb]

Could you please add a test case or test cases that highlight the behavioural change?

[eopXD]

Update code.

[eopXD]

Hi @asb,

I think this patch only addresses the TODO and has no behavioral change.
Test case is already covered under llvm/test/MC/RISCV/target-abi-invalid.s.

[luismarques]

In D115853#3197131, @eopXD wrote:

I think this patch only addresses the TODO and has no behavioral change.

I suggest renaming the title to "[RISCV][NFC] Move ilp32e/rv32e checks to AsmParser and TargetLowering", to make it clearer that this is just a refactoring.

Test case is already covered under llvm/test/MC/RISCV/target-abi-invalid.s.

The new error message in RISCVISelLowering.cpp isn't yet covered by tests but that line currently isn't reachable due to the report_fatal_error in line 52. It's not ideal that this can't have test coverage. @asb thoughts?

[luismarques]

In D115853#3197551, @luismarques wrote:

The new error message in RISCVISelLowering.cpp isn't yet covered by tests but that line currently isn't reachable due to the report_fatal_error in line 52. It's not ideal that this can't have test coverage. @asb thoughts?

I was assuming that it would be a bad idea to remove the report_fatal_error because code generation would still occur (despite the error message) and possibly crash in unpleasant ways but maybe that's not significantly different from the other error conditions above where that also happens.

[jrtc27]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

72 ↗

(On Diff #394798)

This is clearly wrong, just copy-pasted from the RVI cases

[eopXD]

Delete redundant code.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

72 ↗

(On Diff #394798)

Yes you are right. I think the code I added here is redundant. Just deleted.

[eopXD]

Rebase

[asb]

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

55

This code path can be exercised through a test like those in test/CodeGen/RISCV/target-abi-invalid.ll (e.g. targeting riscv32 -mattr=+e and without setting the ABI to ilp32e). With ilp32e ABI support not yet landed but some support for command-line options etc, the current state of affairs is a bit weird. But I think it would be better to add a test case to taret-abi-invalid.ll rather than removing this logic.

[eopXD]

Rebase and add testcase.

[eopXD]

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

55

Yeah, I have added a test for this. We would have to wait for ilp32e to land before this patch.

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

if (ABIName.endswith("f") &&
       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";
+    } else if (!ABIName.endswith("e") &&
+               getSTI().getFeatureBits()[RISCV::FeatureRV32E]) {
+      errs() << "Only the ilp32e ABI is supported for RV32E (ignoring "
+                "target-abi)\n";
     }
 
     const MCObjectFileInfo *MOFI = Parser.getContext().getObjectFileInfo();
     ParserOptions.IsPicEnabled = MOFI->isPositionIndependent();
   }
 };
 
 /// RISCVOperand - Instances of this class represent a parsed machine

llvm/lib/Target/RISCV/MCTargetDesc/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 (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;
 
   // For now, default to the ilp32/ilp32e/lp64 ABI if no explicit ABI is given
   // or an invalid/unrecognised string is given. In the future, it might be
   // worth changing this to default to ilp32f/lp64f and ilp32d/lp64d when

llvm/test/CodeGen/RISCV/target-abi-invalid.ll

 ; RUN: llc -mtriple=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: llc -mtriple=riscv32 -mattr=+e < %s 2>&1 \
+; RUN:   | FileCheck -check-prefix=RV32-ILP32E %s
+
+; RV32-ILP32E: Only the ilp32e ABI is supported for RV32E (ignoring target-abi)
 define void @nothing() nounwind {
   ret void
 }