[PATCH] D93298: [RISCV] add the MC layer support of Zfinx extension
Jessica Clarke via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jan 4 03:47:39 PST 2021
jrtc27 added a comment.
Your tests look like copies of the F/D/Zfh tests with not all the comments updated and instances of tests that just don't make sense for Zfinx. I only skimmed them and picked up a few issues, I haven't gone through them thoroughly, please do that yourself.
================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:341
+ bool isGPRasFPR() const { return isGPR() && IsGPRasFPR; }
+
----------------
as -> As everywhere
================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:975-977
+ // As the parser couldn't differentiate an GPRH, GPRF, GPRD, GPRPD from an
+ // GPR, coerce the register from GPR to these if necessary.
+ if (IsRegGPR && Kind == MCK_GPRPD && !isRV64()) {
----------------
Comment doesn't match what you do, and it's "a GPR" not "an GPR". Are there tests that demonstrate all these potentially-problematic cases as working?
================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1663
+OperandMatchResultTy RISCVAsmParser::parseGPRasFPR(OperandVector &Operands) {
+ switch (getLexer().getKind()) {
----------------
Why can't you just use parseRegister?
================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:403
+ !STI.getFeatureBits()[RISCV::Feature64Bit]) {
+ LLVM_DEBUG(dbgs() << "Trying RV32DZfinx table (Double in Integer and"
+ "rv32)\n");
----------------
RV32Zfdinx?
================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:414-416
+ STI.getFeatureBits()[RISCV::FeatureExtZfhinx] ||
+ (STI.getFeatureBits()[RISCV::FeatureExtZdinx] &&
+ STI.getFeatureBits()[RISCV::Feature64Bit])) {
----------------
Don't all these imply Zfinx? Should just need to check if Zfinx.
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfinx.td:20
+
+def GPRasFPR : AsmOperandClass {
+ let Name = "GPRasFPR";
----------------
I don't see why the default `parseRegister` doesn't work, then you can ditch all these `AsmOperandClass`es.
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfinx.td:35
+
+def GPRHOp : RegisterOperand<GPRH> {
+ let ParserMatchClass = GPRasFPR;
----------------
I don't like that all these exist, but not sure if there's a nice way to avoid them? If not, please use 16/32/64 suffixes like for FPRs rather than H/D/nothing, it gets confusing otherwise.
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfinx.td:59
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+class FPUnaryOpINX_r<bits<7> funct7, bits<3> funct3, RegisterOperand rdty,
+ RegisterOperand rs1ty, string opcodestr>
----------------
Don't duplicate all these, they're identical to the normal floating point versions.
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfinx.td:71
+
+// RV32/64 and zfh extension
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
----------------
Zfh
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfinx.td:119
+
+// RV64 D extension
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
----------------
Zfd
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfinx.td:177
+
+// instructions in zfh extension
+let Predicates = [HasStdExtZfhinx] in {
----------------
Instruction in Zfhinx extension
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfinx.td:177
+
+// instructions in zfh extension
+let Predicates = [HasStdExtZfhinx] in {
----------------
jrtc27 wrote:
> Instruction in Zfhinx extension
It'd be nicer if the file were split up into multiple separate files the same way as we do for F/D/Zfh rather than having a single big file with everything.
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfinx.td:275
+
+// instructions in F extension and rv64
+let Predicates = [HasStdExtZfhinx, IsRV64] in {
----------------
Instructions in Zfhinx extension on RV64 / Instructions in RV64Zfhinx
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfinx.td:316
+
+// instructions in F extension, no matter rv32 or rv64
+let Predicates = [HasStdExtZfinx] in {
----------------
Instructions in Zfinx extension
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfinx.td:415
+
+// instructions that in F extention and rv64
+let Predicates = [HasStdExtZfinx, IsRV64] in {
----------------
etc, I won't keep repeating myself
================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:113
[i32, i64]>;
+def ZfinxLenVT : ValueTypeByHwMode<[RV32, RV64],
+ [f32, f64]>;
----------------
This doesn't seem to be used.
================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:468
+ Reg.AltNames> {
+ let SubRegIndices = [sub_32, sub_32_hi];
+ }
----------------
Does this hard-coding of 32 cause issues on RV64?
================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:501-502
+def GPRPD : RegisterClass<"RISCV", [f64], 64, (add
+ X0_PD, X2_PD, X4_PD, X6_PD, X8_PD, X10_PD, X12_PD, X14_PD, X16_PD,
+ X18_PD, X20_PD, X22_PD, X24_PD, X26_PD, X28_PD, X30_PD)>;
----------------
The order is important for register allocation.
================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:503
+ X18_PD, X20_PD, X22_PD, X24_PD, X26_PD, X28_PD, X30_PD)>;
\ No newline at end of file
----------------
Whitespace
================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:67
MVT XLenVT = MVT::i32;
+ MVT ZfinxLenVT = MVT::i32;
RISCVABI::ABI TargetABI = RISCVABI::ABI_Unknown;
----------------
This doesn't seem to be used, but it should be f32 if it's needed.
================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:97
const RISCVInstrInfo *getInstrInfo() const override { return &InstrInfo; }
- const RISCVRegisterInfo *getRegisterInfo() const override {
- return &RegInfo;
- }
+ const RISCVRegisterInfo *getRegisterInfo() const override { return &RegInfo; }
const RISCVTargetLowering *getTargetLowering() const override {
----------------
Don't change unrelated code
================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:154
};
-} // End llvm namespace
+} // namespace llvm
----------------
Don't change unrelated code
================
Comment at: llvm/test/MC/RISCV/rv32i-invalid.s:120
-# Bare symbol names when an operand modifier is required and unsupported
+# Bare symbol names when an operand modifier is required and unsupported
# operand modifiers.
----------------
Someone should commit this separately
================
Comment at: llvm/test/MC/RISCV/rv32i-invalid.s:176-178
+fadd.h a0, a1, a2 # CHECK: :[[@LINE]]:1: error: instruction requires the following: 'Zfhinx' (Half float in Integer)
+fadd.s a0, a1, a2 # CHECK: :[[@LINE]]:1: error: instruction requires the following: 'Zfinx' (Float in Integer)
+fadd.d a0, a0, a2 # CHECK: :[[@LINE]]:1: error: instruction requires the following: 'Zdinx' (Double in Integer)
----------------
These names should match the F/D/Zfh names, not be abbreviations thereof
================
Comment at: llvm/test/MC/RISCV/rv32zfinx-invalid.s:31
+
+# Using 'D' instructions for an 'F'-only target
+fadd.d t1, t3, t5 # CHECK: :[[@LINE]]:1: error: instruction requires the following: 'Zdinx' (Double in Integer)
----------------
Comment is wrong
================
Comment at: llvm/test/MC/RISCV/rv32zfinx-invalid.s:34
+
+# Using RV64F instructions for RV32 is tested in rv64f-valid.s
----------------
Ditto
================
Comment at: llvm/test/MC/RISCV/rv64zfhinx-invalid.s:4-10
+# FP regs where Integer regs are expected
+fcvt.l.h t0, fa0 # CHECK: :[[@LINE]]:1: error: instruction requires the following: 'Zfh' (Half-Precision Floating-Point)
+fcvt.lu.h t1, fa1 # CHECK: :[[@LINE]]:1: error: instruction requires the following: 'Zfh' (Half-Precision Floating-Point)
+
+# Integer registers where FP regs are expected
+fcvt.h.l fa2, t2 # CHECK: :[[@LINE]]:1: error: instruction requires the following: 'Zfh' (Half-Precision Floating-Point)
+fcvt.h.lu fa3, t3 # CHECK: :[[@LINE]]:1: error: instruction requires the following: 'Zfh' (Half-Precision Floating-Point)
----------------
The comments for these tests don't make any sense for Zfinx
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93298/new/
https://reviews.llvm.org/D93298
More information about the cfe-commits
mailing list