This patch added the MC layer support of Zfinx extension.
Authored-by: StephenFan
Co-Authored-by: Shao-Ce Sun
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Firstly, please generate your diffs with full context (-U with a sufficiently-large number). Secondly, can we avoid having to do a bunch of duplication with some clever use of multiclasses for F/D/Zfh and pseudos? Though maybe it's small enough that the duplication is easier to reason about than an obfuscated abstracted version.
Also, do you not need more predicates? You can't just assume all of F, D and Zfh exist.
As for Zfinx itself, well, the idea is fine, but I really detest the way it's being done as an extension to F/D/Zfh. Running F code on an FZfh core _does not work_ so it is not an _extension_. Instead it should really be a set of separate extensions to I/E that conflict with F/D/Zfh, i.e. Zfinx, Zdinx and Zfhinx, but apparently asking code that complies with a ratified standard to change itself in order to not break when a new extension is introduced is a-ok in the RISC-V world.
| llvm/test/MC/RISCV/rv32zfh-invalid.s | ||
|---|---|---|
| 33 ↗ | (On Diff #311893) | These need to stay, but presumably instead as errors about Zfinx not being enabled? |
| clang/lib/Basic/Targets/RISCV.h | ||
|---|---|---|
| 40 ↗ | (On Diff #311893) | We really should just initialize these to false at their declarations so you don't have to change the constructor initializer list every time one is added.. I might submit a patch for that. |
Comment Actions
Do you have implement register pair for rv32ifd_zfinx? I didn't saw the related implementation, but I could be wrong since I am not LLVM expert, in case you have implemented, you need a test case for that.
| llvm/lib/Target/RISCV/RISCVInstrInfoZfinx.td | ||
|---|---|---|
| 103 | use GPR as instruction operand may cause the codegen part of zfinx report errors. Because the GPR has data type i32 or i64, However, the zfinx will deal with the data type f64 or f32. | |
| llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
|---|---|---|
| 977 | It seems like that this function is not useful. | |
Comment Actions
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.
| llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
|---|---|---|
| 341 | as -> As everywhere | |
| 969–971 | 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? | |
| 1648 | Why can't you just use parseRegister? | |
| llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp | ||
| 403 | RV32Zfdinx? | |
| 414–416 | Don't all these imply Zfinx? Should just need to check if Zfinx. | |
| llvm/lib/Target/RISCV/RISCVInstrInfoZfinx.td | ||
| 21 | I don't see why the default parseRegister doesn't work, then you can ditch all these AsmOperandClasses. | |
| 36 | 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. | |
| 60 | Don't duplicate all these, they're identical to the normal floating point versions. | |
| 72 | Zfh | |
| 120 | Zfd | |
| 178 | Instruction in Zfhinx extension | |
| 178 | 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. | |
| 276 | Instructions in Zfhinx extension on RV64 / Instructions in RV64Zfhinx | |
| 317 | Instructions in Zfinx extension | |
| 416 | etc, I won't keep repeating myself | |
| llvm/lib/Target/RISCV/RISCVRegisterInfo.td | ||
| 113 | This doesn't seem to be used. | |
| 466 | Does this hard-coding of 32 cause issues on RV64? | |
| 499–500 | The order is important for register allocation. | |
| 506 | Whitespace | |
| llvm/lib/Target/RISCV/RISCVSubtarget.h | ||
| 67 | This doesn't seem to be used, but it should be f32 if it's needed. | |
| 98 | Don't change unrelated code | |
| 155 | Don't change unrelated code | |
| llvm/test/MC/RISCV/rv32i-invalid.s | ||
| 120 | Someone should commit this separately | |
| 176–178 | These names should match the F/D/Zfh names, not be abbreviations thereof | |
| llvm/test/MC/RISCV/rv32zfinx-invalid.s | ||
| 32 | Comment is wrong | |
| 35 | Ditto | |
| llvm/test/MC/RISCV/rv64zfhinx-invalid.s | ||
| 5–11 | The comments for these tests don't make any sense for Zfinx | |
| llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
|---|---|---|
| 1648 | use the default parseRegister will make the test cases in other files fail. For example: fcvt.d.l a3, ft3 # CHECK: :[[@LINE]]:10: error: invalid operand for instruction this is the test case in rv64d-invalid.s. If uses the default parseRegister. the invalid operand is in column 14 (ft3 operand) instead of 10 (a3 operand). | |
| llvm/lib/Target/RISCV/RISCVInstrInfoZfinx.td | ||
| 60 | Because of normal floating point version only support RegisterClass, but we use the RegisterOperand, so we change this. Or if there is more convenient way to resolve this? | |
| llvm/lib/Target/RISCV/RISCVRegisterInfo.td | ||
| 466 | I don't known if it will cause issues on RV64. But the zfinx spec specifies that register pairs are only used in RV32 | |
| llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
|---|---|---|
| 1648 |
use the default parseRegister will make the test cases in other files fail. For example: fcvt.d.l a3, ft3 # CHECK: :[[@LINE]]:10: error: invalid operand for instruction this is the test case in rv64d-invalid.s. If uses the default parseRegister. the invalid operand is in column 14 (ft3 operand) instead of 10 (a3 operand). | |
| llvm/lib/Target/RISCV/RISCVInstrInfoZfinx.td | ||
| 60 |
Because of normal floating point version only support RegisterClass, but we use the RegisterOperand, so we change this. Or if there is more convenient way to resolve this? | |
| llvm/lib/Target/RISCV/RISCVRegisterInfo.td | ||
| 466 |
I don't known if it will cause issues on RV64. But the zfinx spec specifies that register pairs are only used in RV32 | |
| llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
|---|---|---|
| 944 | Use "% 2 == 0" since the next line uses "/ 2" | |
Comment Actions
I started reviewing this alongside the specification in https://github.com/riscv/riscv-zfinx/blob/master/Zfinx_spec.adoc. At the time of writing, it seems to define "zfinx" but not "zfhinx" and "zfdinx" as seem to be used in this patch. I think intent is that rv32ifd_zfinx is the equivalent of "zfdinx" in this patch. Is there a reason to go for different naming, or a different version of the spec I should be looking at?
Comment Actions
According to @jrtc27 's review that is
"As for Zfinx itself, well, the idea is fine, but I really detest the way it's being done as an extension to F/D/Zfh. Running F code on an FZfh core _does not work_ so it is not an _extension_. Instead it should really be a set of separate extensions to I/E that conflict with F/D/Zfh, i.e. Zfinx, Zdinx and Zfhinx, but apparently asking code that complies with a ratified standard to change itself in order to not break when a new extension is introduced is a-ok in the RISC-V world.".
We split the Zfinx into 3 separate extensions which is Zfinx, Zdinx, and Zfhinx.
Comment Actions
Ah I see. I interpreted jrtc27's comment as a general gripe about the spec (which perhaps could be relayed to those working on the zfinx spec) rather as a direction for changing this patch in particular. Anyway, it's a detail that shouldn't affect an initial review. Thanks for clarifying.
Comment Actions
Well, it was "I'm uneasy about accepting a patch adding an extension that is fundamentally flawed in its current form" (unlike some of the others where they're subject to change but don't _break_ anything).
Comment Actions
Oh, I'm sorry. It seems that I misunderstood @jrtc27 's comment. I will merge the Zfinx, Zdinx, Zfhinx into Zfinx if this patch is ready for accepting.