Page MenuHomePhabricator

[RISCV] add the MC layer support of Zfinx extension
Needs ReviewPublic

Authored by achieveartificialintelligence on Dec 15 2020, 6:53 AM.

Details

Summary

This patch added the MC layer support of Zfinx extension.

Authored-by: StephenFan
Co-Authored-by: Shao-Ce Sun

Diff Detail

Event Timeline

achieveartificialintelligence requested review of this revision.Dec 15 2020, 6:53 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 15 2020, 6:53 AM

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?

craig.topper added inline comments.Dec 15 2020, 11:43 AM
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.

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.

StephenFan added inline comments.
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.

achieveartificialintelligence marked 2 inline comments as done.

Including Zfinx, zdinx. Zfhinx.

StephenFan added inline comments.Jan 4 2021, 3:20 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
977

It seems like that this function is not useful.

achieveartificialintelligence marked an inline comment as done.Jan 4 2021, 3:33 AM

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.

We have solved this now.

jrtc27 added a comment.Jan 4 2021, 3:47 AM

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

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.

Thanks for your advices. We will solve the issues you have mentioned above.

StephenFan added inline comments.Jan 4 2021, 4:19 AM
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

StephenFan added inline comments.Jan 4 2021, 4:23 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1648

Why can't you just use parseRegister?

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?

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

Does this hard-coding of 32 cause issues on RV64?

I don't known if it will cause issues on RV64. But the zfinx spec specifies that register pairs are only used in RV32

craig.topper added inline comments.Jan 4 2021, 4:13 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
944

Use "% 2 == 0" since the next line uses "/ 2"

achieveartificialintelligence marked 5 inline comments as done.

Thanks for suggestions. We updated the code again.

asb added a comment.Fri, Feb 5, 2:27 AM

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?

In D93298#2544443, @asb wrote:

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?

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.

asb added a comment.Fri, Feb 5, 5:14 AM

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.

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.

jrtc27 added a comment.Fri, Feb 5, 5:16 AM
In D93298#2544775, @asb wrote:

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.

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.

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).

In D93298#2544775, @asb wrote:

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.

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.

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.