[RISCV][MC] Add support for experimental zfa extension(FLI instruction not included)

[joshua-arch1]

Summary

This implements experimental support for the RISCV Zfa extension as specified here: https://github.com/riscv/riscv-isa-manual/releases/download/draft-20221119-5234c63/riscv-spec.pdf, Ch. 25. This extension has not been ratified. Once ratified, it'll move out of experimental status.

This change only adds assembly support for instructions except load-immediate instructions (fli.s/fli.d/fli.h).

[craig.topper]

Please update llvm/docs/RISCVUsage.rst

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td

11

The spec says 0.1 I think.

32

I don't think you want to pass FINX here. That will create additional versions that use GPR has registers like Zfinx.

I think you should use FPALU_rr instead.

Similar issues exist for other instructions.

llvm/test/MC/RISCV/rv32zfa-only-valid.s

2

Please add -invalid tests as well to make sure these instructions are not accepted without Zfa.

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td

50

We need to only allow rtz for the rounding mode for fcvtmod.w.d.

[craig.topper]

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

538 ↗

(On Diff #493208)

Please revisit this. I rewrote how isFRMArg works in D142833

[reames]

Drive by comment only, deferring to @craig.topper for most of this.

llvm/docs/RISCVUsage.rst

160 ↗

(On Diff #493265)

...implements a subset of the...

(i.e. give a clue the implementation is not complete)

[craig.topper]

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

1859 ↗

(On Diff #493462)

Can we reuse parseFRMArg and check the value in isRTZArg()? Add a DiagnosticType to RTZArg and diagnose in RISCVAsmParser::MatchAndEmitInstruction

llvm/test/MC/RISCV/rv32zfa-valid.s

39

Need tests that the "dyn" argument is optional. This used to require InstAliases to be added to the .td file, but in the last 24 hours I modified FRMArg to be an Optional operand. See D142959

[jrtc27]

Please remove the [n/4] and links to other revisions from the summary, that's what the related revisions feature is for (that I just fixed up for you, as it's a pain not to have that and be able to see at a glance what the dependencies are).

Sidenote: I don't see a [4/4] patch?...

[reames]

A collection of minor comments. I did not closely examine the binary encodings.

This review is missing changes to:
clang/test/Preprocessor/riscv-target-features.c
test/CodeGen/RISCV/attributes.ll
test/MC/RISCV/attribute-arch.s

(These are just boilerplate for the new extension name.)

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1868

Alphabetical please.

llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td

59

This is a purely optional comment.

You have the instructions grouped by data type. It would make the definitions easier to follow if you instead grouped by instruction name. This would involve slightly more predicate scopes, but would more directly match the wording in the specification.

67

All of these are under Zfa, you can use an outer scope for this and an inner scope for the floating point extensions.

llvm/test/MC/RISCV/rv32zfa-valid.s

2

This file should be zfa-valid.s (i.e. remove the rv32) since it tests both rv32 and rv64.

47

If I'm reading the spec correctly, other rounding modes are valid on this instruction right? If so, would you mind adding at least one other rounding mode example? There's another instruction in this own set where only rtz is valid, and having a test which clearly distinguishes them would help readability.

111

We need a zfa-invalid.s example here with a different rounding mode. (Since this instruction only allows rtz).

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td

59

I've been wondering if it makes sense to have this file at all or should we fold the instructions into the Zfh, F, and D files?

[reames]

llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td

59

Either would be reasonable. Happy to defer to @craig.topper 's judgement here.

[craig.topper]

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

1859 ↗

(On Diff #493462)

This only seems to have been partially done?

[joshua-arch1]

Ping.

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

1859 ↗

(On Diff #493462)

I think if we reuse parseFRMArg and check the value in isRTZArg(), the code quantity will be larger, since we need to add extra logic in isRTZArg() like what you did in initial isFRMArg() implementation.

1859 ↗

(On Diff #493462)

Since we have moved FRM parsing to a custom operand parser, it's briefer and more consistent to check all types of FRM operands in operand parsers instead of in isFRMArg()/isRTZArg().

1859 ↗

(On Diff #493462)

This only seems to have been partially done?

Did you mean 'reuse parseFRMArg and check the value in isRTZArg()'? I didn't do that. You can check my reply to your last comment.

[craig.topper]

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

1859 ↗

(On Diff #493462)

Yes.

We can use parseFRMArg for parsing. And in isRTZArg() check `isFRMArg() && FRM.FRM == RISCVFPRndMode::RTZ;'

[joshua-arch1]

Ping.

[craig.topper]

This comment was removed by craig.topper.

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td

68

Lists in nested let statements don't concatenate. This list needs to be [HasStdExtD, HasStdExtZfa]

[craig.topper]

llvm/test/MC/RISCV/zfa-valid.s

12 ↗

(On Diff #495018)

Please add a CHECK-NO-EXT RUN like some of the other tests to make sure the predicates are correct.

[joshua-arch1]

Ping.

llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td

68

So I'm wondering if we should use nested format as @reames mentioned in the last comment. I guess lists in order will be better.

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td

68

Yeah lists in order will be better.

[craig.topper]

llvm/test/MC/RISCV/rv32zfa-only-valid.s

7

Add CHECK-NO-EXT here too

llvm/test/MC/RISCV/zfa-valid.s

12 ↗

(On Diff #495759)

If we remove the +d and +zfh here do we get error messages that mention all of the required extensions? I want to make sure we check that the .h instructions require zfh and zfa. Same for .d

[craig.topper]

LGTM

[asb]

llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td

59

No strong opinion for on keeping this separate or merging into another file. At least for the G extensions, the instruction definitions are in the order they're listed in the RV32/64G Instruction Set Listings table, on the basis this makes it easier to check at a glance if the right encodings are used. Sadly a similar table doesn't seem to exist in the documentation for Zfa.

[craig.topper]

@joshua-arch1 do you have commit access or do you need me to merge this?

[joshua-arch1]

In D141984#4124997, @craig.topper wrote:

@joshua-arch1 do you have commit access or do you need me to merge this?

I have no access yet. Could you please help merge this?

[joshua-arch1]

In D141984#4125398, @joshua-arch1 wrote:

In D141984#4124997, @craig.topper wrote:

@joshua-arch1 do you have commit access or do you need me to merge this?

I have no access yet. Could you please help merge this?

I have been granted commit access recently and I merged this patch just now.

[reames]

In D141984#4125441, @joshua-arch1 wrote:

In D141984#4125398, @joshua-arch1 wrote:

In D141984#4124997, @craig.topper wrote:

@joshua-arch1 do you have commit access or do you need me to merge this?

I have no access yet. Could you please help merge this?

I have been granted commit access recently and I merged this patch just now.

I just checked, and I do *not* see this change in tree. Did you forget to push? Or merge to the wrong repo?

[reames]

This was initially landed in two commits without proper commit messages. I reverted both and relanded the change this morning.

Original commits:

commit 321cd52ba2647259f58b0d38cdb62528a9ded9a1
Author: Jun Sha (Joshua) <[e-mail]>
Date: Thu Feb 16 09:54:40 2023 +0800

Update: [RISCV][MC] Add support for experimental zfa extension(FLI instruction not included)

commit 54c136e6c630966255293d42c882eab116437834
Author: Jun Sha (Joshua) <[e-mail]>
Date: Thu Feb 16 09:50:59 2023 +0800

[RISCV][MC] Add support for experimental zfa extension(FLI instruction not included)

[reames]

Seeing buildbot failures, which look valid. Example:
https://lab.llvm.org/buildbot#builders/196/builds/26601

This caused me to look more closely at the change, and we are definitely missing things which should be enabled by such an extension change. One specific example is that we failed to add zfa in SupportedExperimentalExtensions.

Oddly, I do not see this failure locally. I don't have a good explanation for that.

I'm about to revert.

[reames]

In D141984#4132433, @reames wrote:

Oddly, I do not see this failure locally. I don't have a good explanation for that.

I chatted w/ @craig.topper, and he confirmed that this didn't reproduce locally for him either. Neither of us have a good explanation of why right now. I need to run for the moment, but plan to return to this later in the day to see if we can get this straightened out.

My guess earlier about RISCVISAInfo.cpp is probably not the issue. Craig mentioned that the llvm-mc feature detection should be driven from tablegen, which this change did include.

llvm/lib/Target/RISCV/RISCV.td

def HasStdExtZhinx : Predicate<"Subtarget->hasStdExtZhinx()">,
                                "'Zhinx' (Half Float in Integer)">;
 
 def HasStdExtZhinxOrZhinxmin
     : Predicate<"Subtarget->hasStdExtZhinx() || Subtarget->hasStdExtZhinxmin()">,
                 AssemblerPredicate<(any_of FeatureStdExtZhinx, FeatureStdExtZhinxmin),
                                    "'Zhinx' (Half Float in Integer) or "
                                    "'Zhinxmin' (Half Float in Integer Minimal)">;
 
+def FeatureStdExtZfa
+    : SubtargetFeature<"experimental-zfa", "HasStdExtZfa", "true",
+                       "'Zfa' (Additional Floating-Point)",
+                       [FeatureStdExtF]>;
+def HasStdExtZfa : Predicate<"Subtarget->hasStdExtZfa()">,
+                             AssemblerPredicate<(all_of FeatureStdExtZfa),
+                             "'Zfa' (Additional Floating-Point)">;
+                                                         
 def FeatureStdExtC
     : SubtargetFeature<"c", "HasStdExtC", "true",
                        "'C' (Compressed Instructions)">;
 def HasStdExtC : Predicate<"Subtarget->hasStdExtC()">,
                            AssemblerPredicate<(all_of FeatureStdExtC),
                            "'C' (Compressed Instructions)">;
 
 def FeatureStdExtZba

llvm/lib/Target/RISCV/RISCVInstrInfo.td

 include "RISCVInstrInfoA.td"
 include "RISCVInstrInfoF.td"
 include "RISCVInstrInfoD.td"
 include "RISCVInstrInfoC.td"
 include "RISCVInstrInfoZb.td"
 include "RISCVInstrInfoZk.td"
 include "RISCVInstrInfoV.td"
 include "RISCVInstrInfoZfh.td"
+include "RISCVInstrInfoZfa.td"
 include "RISCVInstrInfoZicbo.td"
 
 //===----------------------------------------------------------------------===//
 // Vendor extensions
 //===----------------------------------------------------------------------===//
 
 include "RISCVInstrInfoXVentana.td"
 include "RISCVInstrInfoXTHead.td"

llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td

+//===-- RISCVInstrInfoZfa.td - RISC-V 'Zfa' instructions ---*- tablegen -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file describes the RISC-V instructions from the standard 'Zfa'
+// additional floating-point extension, version 1.0.
+// This version is still experimental as the 'Zfa' extension hasn't been
+// ratified yet.
+//
+//===----------------------------------------------------------------------===//
+
+//===----------------------------------------------------------------------===//
+// Instruction class templates
+//===----------------------------------------------------------------------===//
+
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0, mayRaiseFPException = 1 in
+class FPBinaryOp_rr<bits<7> funct7, bits<3> funct3, DAGOperand rdty, 
+                    DAGOperand rsty, string opcodestr>
+    : RVInstR<funct7, funct3, OPC_OP_FP, (outs rdty:$rd), 
+              (ins rsty:$rs1, rsty:$rs2), opcodestr, "$rd, $rs1, $rs2">;
+
+//===----------------------------------------------------------------------===//
+// Instructions
+//===----------------------------------------------------------------------===//
+
+let Predicates = [HasStdExtZfa] in {
+defm FMINM_S: FPALU_rr_m<0b0010100, 0b010, "fminm.s", FINX, /*Commutable*/ 1>;
+defm FMAXM_S: FPALU_rr_m<0b0010100, 0b011, "fmaxm.s", FINX, /*Commutable*/ 1>;
+
+defm FROUND_S : FPUnaryOp_r_frm_m<0b0100000, 0b00100, FFINX, "fround.s">;
+defm FROUNDNX_S : FPUnaryOp_r_frm_m<0b0100000, 0b00101, FFINX, "froundnx.s">;
+
+defm FLTQ_S : FPCmp_rr_m<0b1010000, 0b101, "fltq.s", FINX>;
+defm FLEQ_S : FPCmp_rr_m<0b1010000, 0b100, "fleq.s", FINX>;
+} // Predicates = [HasStdExtZfa]
+
+let Predicates = [HasStdExtZfa, HasStdExtD] in {
+defm FMINM_D: FPALU_rr_m<0b0010101, 0b010, "fminm.d", DINX, /*Commutable*/ 1>;
+defm FMAXM_D: FPALU_rr_m<0b0010101, 0b011, "fmaxm.d", DINX, /*Commutable*/ 1>;
+
+defm FROUND_D : FPUnaryOp_r_frm_m<0b0100001, 0b00100, DDINX, "fround.d">;
+defm FROUNDNX_D : FPUnaryOp_r_frm_m<0b0100001, 0b00101, DDINX, "froundnx.d">;
+
+defm FCVTMOD_W_D
+    : FPUnaryOp_r_frm_m<0b1100001, 0b01000, XDINX, "fcvtmod.w.d">,
+      Sched<[WriteFCvtF64ToI32, ReadFCvtF64ToI32]>;
+
+defm FLTQ_D : FPCmp_rr_m<0b1010001, 0b101, "fltq.d", DINX>;
+defm FLEQ_D : FPCmp_rr_m<0b1010001, 0b100, "fleq.d", DINX>;
+} // Predicates = [HasStdExtZfa, HasStdExtD]
+
+let Predicates = [HasStdExtZfa, HasStdExtD, IsRV32] in {
+def FMVH_X_D : FPUnaryOp_r<0b1110001, 0b00001, 0b000, GPR, FPR64, "fmvh.x.d">,
+               Sched<[WriteFMovF32ToI32, ReadFMovF32ToI32]>;
+def FMVP_D_X : FPBinaryOp_rr<0b1011001, 0b000, FPR64, GPR, "fmvp.d.x">,
+               Sched<[WriteFMovI32ToF32, ReadFMovI32ToF32]>;
+} // Predicates = [HasStdExtZfa, HasStdExtD, IsRV32]
+
+let Predicates = [HasStdExtZfa, HasStdExtZfh] in {
+defm FMINM_H: FPALU_rr_m<0b0010110, 0b010, "fminm.h", HINX, /*Commutable*/ 1>;
+defm FMAXM_H: FPALU_rr_m<0b0010110, 0b011, "fmaxm.h", HINX, /*Commutable*/ 1>;
+
+defm FROUND_H : FPUnaryOp_r_frm_m<0b0100010, 0b00100, HHINX, "fround.h">;
+defm FROUNDNX_H : FPUnaryOp_r_frm_m<0b0100010, 0b00101, HHINX, "froundnx.h">;
+
+defm FLTQ_H : FPCmp_rr_m<0b1010010, 0b101, "fltq.h", HINX>;
+defm FLEQ_H : FPCmp_rr_m<0b1010010, 0b100, "fleq.h", HINX>;
+} // Predicates = [HasStdExtZfa, HasStdExtZfh]
 No newline at end of file

llvm/test/MC/RISCV/rv32zfa-only-valid.s

+# RUN: llvm-mc %s -triple=riscv32 -mattr=+experimental-zfa,+d,+zfh -riscv-no-aliases -show-encoding \
+# RUN:     | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=+experimental-zfa,+d,+zfh < %s \
+# RUN:     | llvm-objdump --mattr=+experimental-zfa,+d,+zfh -M no-aliases -d -r - \
+# RUN:     | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+
+# CHECK-ASM-AND-OBJ: fmvh.x.d a1, fs1
+# CHECK-ASM: encoding: [0xd3,0x85,0x14,0xe2]
+fmvh.x.d a1, fs1
+
+# CHECK-ASM-AND-OBJ: fmvp.d.x fs1, a1, a2
+# CHECK-ASM: encoding: [0xd3,0x84,0xc5,0xb2]
+fmvp.d.x fs1, a1, a2

llvm/test/MC/RISCV/rv32zfa-valid.s

+# RUN: llvm-mc %s -triple=riscv32 -mattr=+experimental-zfa,+d,+zfh -riscv-no-aliases -show-encoding \
+# RUN:     | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc %s -triple=riscv64 -mattr=+experimental-zfa,+d,+zfh -riscv-no-aliases -show-encoding \
+# RUN:     | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=+experimental-zfa,+d,+zfh < %s \
+# RUN:     | llvm-objdump --mattr=+experimental-zfa,+d,+zfh -M no-aliases -d -r - \
+# RUN:     | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+experimental-zfa,+d,+zfh < %s \
+# RUN:     | llvm-objdump --mattr=+experimental-zfa,+d,+zfh -M no-aliases -d -r - \
+# RUN:     | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+
+# CHECK-ASM-AND-OBJ: fminm.s fa0, fa1, fa2
+# CHECK-ASM: encoding: [0x53,0xa5,0xc5,0x28]
+fminm.s fa0, fa1, fa2
+
+# CHECK-ASM-AND-OBJ: fmaxm.s fs3, fs4, fs5
+# CHECK-ASM: encoding: [0xd3,0x39,0x5a,0x29]
+fmaxm.s fs3, fs4, fs5
+
+# CHECK-ASM-AND-OBJ: fminm.d fa0, fa1, fa2
+# CHECK-ASM: encoding: [0x53,0xa5,0xc5,0x2a]
+fminm.d fa0, fa1, fa2
+
+# CHECK-ASM-AND-OBJ: fmaxm.d fs3, fs4, fs5
+# CHECK-ASM: encoding: [0xd3,0x39,0x5a,0x2b]
+fmaxm.d fs3, fs4, fs5
+
+# CHECK-ASM-AND-OBJ: fminm.h fa0, fa1, fa2
+# CHECK-ASM: encoding: [0x53,0xa5,0xc5,0x2c]
+fminm.h fa0, fa1, fa2
+
+# CHECK-ASM-AND-OBJ: fmaxm.h fs3, fs4, fs5
+# CHECK-ASM: encoding: [0xd3,0x39,0x5a,0x2d]
+fmaxm.h fs3, fs4, fs5
+
+# CHECK-ASM-AND-OBJ: fround.s fs1, fs2, dyn
+# CHECK-ASM: encoding: [0xd3,0x74,0x49,0x40]
+fround.s fs1, fs2, dyn
+
+# CHECK-ASM-AND-OBJ: froundnx.s fs1, fs2, dyn
+# CHECK-ASM: encoding: [0xd3,0x74,0x59,0x40]
+froundnx.s fs1, fs2, dyn
+
+# CHECK-ASM-AND-OBJ: fround.d fs1, fs2, dyn
+# CHECK-ASM: encoding: [0xd3,0x74,0x49,0x42]
+fround.d fs1, fs2, dyn
+
+# CHECK-ASM-AND-OBJ: froundnx.d fs1, fs2, dyn
+# CHECK-ASM: encoding: [0xd3,0x74,0x59,0x42]
+froundnx.d fs1, fs2, dyn
+
+# CHECK-ASM-AND-OBJ: fround.h ft1, fa1, dyn
+# CHECK-ASM: encoding: [0xd3,0xf0,0x45,0x44]
+fround.h ft1, fa1, dyn
+
+# CHECK-ASM-AND-OBJ: froundnx.h ft1, fa1, dyn
+# CHECK-ASM: encoding: [0xd3,0xf0,0x55,0x44]
+froundnx.h ft1, fa1, dyn
+
+# CHECK-ASM-AND-OBJ: fcvtmod.w.d a1, ft1, rtz
+# CHECK-ASM: encoding: [0xd3,0x95,0x80,0xc2]
+fcvtmod.w.d a1, ft1, rtz
+
+# CHECK-ASM-AND-OBJ: fltq.s a1, fs1, fs2
+# CHECK-ASM: encoding: [0xd3,0xd5,0x24,0xa1]
+fltq.s a1, fs1, fs2
+
+# CHECK-ASM-AND-OBJ: fleq.s a1, ft1, ft1
+# CHECK-ASM: encoding: [0xd3,0xc5,0x10,0xa0]
+fleq.s a1, ft1, ft1
+
+# CHECK-ASM-AND-OBJ: fltq.d a1, fs1, fs2
+# CHECK-ASM: encoding: [0xd3,0xd5,0x24,0xa3]
+fltq.d a1, fs1, fs2
+
+# CHECK-ASM-AND-OBJ: fleq.d a1, ft1, ft2
+# CHECK-ASM: encoding: [0xd3,0xc5,0x20,0xa2]
+fleq.d a1, ft1, ft2
+
+# CHECK-ASM-AND-OBJ: fltq.h a1, fs1, fs2
+# CHECK-ASM: encoding: [0xd3,0xd5,0x24,0xa5]
+fltq.h a1, fs1, fs2
+
+# CHECK-ASM-AND-OBJ: fleq.h a1, ft1, ft2
+# CHECK-ASM: encoding: [0xd3,0xc5,0x20,0xa4]
+fleq.h a1, ft1, ft2