[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