[RISCV][MC] Add FLI instruction support for the experimental zfa extension

[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 adds assembly support for load-immediate instructions (fli.s/fli.d/fli.h). The assembly prefers decimal constants in C-like syntax. In my implementation, an integer encoding ranging from 0 to 31 can also be accepted, but for the MCInst printer, the constant is specified in decimal notation by default.

[craig.topper]

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

1601

Do we need to check if the Token is AsmToken::Real?

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

355

const

355

If you combined Exp_arr and Man_arr into an array of std::pair is it the same array as LoadFPImmArr from getLoadFPImm?

363

const

383

as an 5-bit -> as a 5-bit

393

const

412

No need for else since the if body returned

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

457 ↗

(On Diff #485930)

Are you enforcing that only rtz is valid for this instruction?

[craig.topper]

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

1 ↗

(On Diff #485054)

I see two test files for zfhmin, rv32zfhmin-invalid.s and rv32zfhmin-valid.s. They contain riscv32 and riscv64 RUN lines.

For zfh. At least for the -valid tests. I see rv32zfh-valid.s and rv64zfh-valid.s. The rv32zfh-valid.s file contains riscv32 and riscv64 RUN lines. The rv64zfh-valid.s contains a riscv64 RUN line and instructionst that are specific to riscv64.

[craig.topper]

I think according to the spec, we need to support parsing "Minimum positive normal" in decimal form. The exact value for is different for fli.h, fli.s, and fli.d.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp

164

I think we should use APFloat::toString here to print with full precision.

[craig.topper]

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

1580

Rather than converting to FPImm here. I think maybe should store the 5-bit immediate in the Operands array. This would require validating the floating point values and converting to integers in this function instead of in addFPImmOperands.

[craig.topper]

Can we split this into two patches? One for fli and one for everything else. I think everything but fli is ready to go in. Split the CodeGen patch the same way. That way we can get some stuff committed.

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

1580

I guess we shouldn’t do that because we need may need multiple isFPImm to handle the different min values for the different sizes.

[reames]

This change is missing a few required bits:

• An update to RISCVUsage.rst
• The attribute tests in clang and llvm. (See https://reviews.llvm.org/D133239 for an example.)

I agree with @craig.topper that splitting this into two patches makes sense. Let's leverage experimental status to make our lives easier.

llvm/lib/Target/RISCV/RISCV.td

130 ↗

(On Diff #486166)

Description string is wrong here.

[joshua-arch1]

In D140460#4050322, @craig.topper wrote:

I think according to the spec, we need to support parsing "Minimum positive normal" in decimal form. The exact value for is different for fli.h, fli.s, and fli.d.

The spec says 'the preferred assembly syntax for entry 1 is min' and 'for entry 1, the assembler will accept decimal constants in C-like syntax'. Are there any conflicts between these two statements? For a certain assembly output of minimum positive normal, what output do we need to get?

[joshua-arch1]

In D140460#4050322, @craig.topper wrote:

I think according to the spec, we need to support parsing "Minimum positive normal" in decimal form. The exact value for is different for fli.h, fli.s, and fli.d.

I think there will be some difficulties in expressing 'minimum positive normal' in decimal form. For single-presicion minimum positive normal, the value is 1.1754943508222875079687365372222456778186655567720875215087517062784172594547271728515625E-38. For double-precison, the value will need to be expressed in a much larger precision even if we use toString().

llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp

164

What formatprecision should I choose for the second parameter in toString()?

[craig.topper]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp

164

I think the default value will print whatever precision is needed.

[craig.topper]

In D140460#4055333, @joshua-arch1 wrote:

In D140460#4050322, @craig.topper wrote:

I think according to the spec, we need to support parsing "Minimum positive normal" in decimal form. The exact value for is different for fli.h, fli.s, and fli.d.

I think there will be some difficulties in expressing 'minimum positive normal' in decimal form. For single-presicion minimum positive normal, the value is 1.1754943508222875079687365372222456778186655567720875215087517062784172594547271728515625E-38. For double-precison, the value will need to be expressed in a much larger precision even if we use toString().

The disassembler should print "min", but the parser should accept the full value.

[joshua-arch1]

In D140460#4059422, @craig.topper wrote:

In D140460#4055333, @joshua-arch1 wrote:

In D140460#4050322, @craig.topper wrote:

I think according to the spec, we need to support parsing "Minimum positive normal" in decimal form. The exact value for is different for fli.h, fli.s, and fli.d.

I think there will be some difficulties in expressing 'minimum positive normal' in decimal form. For single-presicion minimum positive normal, the value is 1.1754943508222875079687365372222456778186655567720875215087517062784172594547271728515625E-38. For double-precison, the value will need to be expressed in a much larger precision even if we use toString().

The disassembler should print "min", but the parser should accept the full value.

In my implementation, the parser can already accept the full value of 'min' as long as we give the immediate with the exact precision.

[joshua-arch1]

Ping.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp

164

Since we only need to print 'min' in the dissassembler, I don't think it neccessary to print with full precision. This issue doesn't exist for other values.

[asb]

Minor note: could you please fix the title of this revision? Perhaps "[RISCV][MC] Add fli instruction support for the experimental zfa extension". Thanks!

[joshua-arch1]

Ping.

Could you please continue to review the FLI patch since the assembly for other instructions is ready to land? @craig.topper

[craig.topper]

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

1567

Capitalize isNegative

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

378

Use uint32_t for the result to make it explicitly 32 bits

397

"as an" -> "as a"

400

You can use Imm.extractBitsAsZExtValue to extract the fields.

414

Use extractBitsAsZExtValue

llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp

161

I'd prefer we print "inf" and "nan" explicitly here and not rely on raw_ostream's printer. Especially for infinity since "INF" is capitalized while "min" and "nan" are lower case.

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

154 ↗

(On Diff #493505)

no test for the minimum written as an FP literal. Does the code accept the correct value or does it accept the single precision minimum value for the double precision instruction?

[craig.topper]

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

297

Is this IsRV64 field used ever?

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

197

This isn't the correct min value for double precision

[craig.topper]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

415

You can't use only 3 bits of the mantissa to match an immediate for isFPImmLegal in the other patch. It has to be bit exact for the entire 64 bit value.

[joshua-arch1]

Ping.

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

297

It isn't used yet. I added it just in order to keep consistent with ImmOp.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

415

In the encoding table, only the top 3 bit of the mantissa differs. We don't need to use all the bits to distinguish between different values. 3-bit expression will not influence precision.

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

197

I think we need to keep consistent with GNU. https://github.com/riscv/riscv-isa-manual/issues/980
GNU prefers to accept 'min' instead of specific values since these values are difficult to express.

[craig.topper]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

415

This function is used by CodeGen in your other patch. That must check all bits of the value. Looking at only 3 bits aliases many distinct values together. This is why your patch accepted ret double 0x102F3E9DF9CF94 as "min" when it should only accept double 0x0010000000000000.

[craig.topper]

This patch accepts this fli.d ft1, 1.1754943508222875079687365372222456778186655567720875215087517062784172594547271728515625e-38 as fli.d ft1, min which is incorrect for double.

[joshua-arch1]

In D140460#4138082, @craig.topper wrote:

This patch accepts this fli.d ft1, 1.1754943508222875079687365372222456778186655567720875215087517062784172594547271728515625e-38 as fli.d ft1, min which is incorrect for double.

Any suggestions on how to address this issue? In my implementation, the values in double precision will be converted to single precsion. This issue won't exist for other values, but for "min", the value will be modified.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

415

So it will only affect 'min'? I think there will be no problems for other values if I only use 3 bits.

415

In the binary representation of double 'min' value, the mantissa bits to be 0. That is to say, the first 3 bits are all 0. I'm still wondering why I cannot use only 3 bits to distinguish between other values in double precision.
If I limit the other bits apart from the first 3 to be 0, I think it can differentiate all the values.

415

I have added some logic to check the other bits of the mantissa. Apart from the first 3 bits, the other bits in the encoding table needs to be all 0.

I tested my CodeGen patch with 0x0010000000000000 and it passed. And as expected, 0x102F3E9DF9CF94 cannot work now.

[joshua-arch1]

In D140460#4138160, @joshua-arch1 wrote:

In D140460#4138082, @craig.topper wrote:

This patch accepts this fli.d ft1, 1.1754943508222875079687365372222456778186655567720875215087517062784172594547271728515625e-38 as fli.d ft1, min which is incorrect for double.

Any suggestions on how to address this issue? In my implementation, the values in double precision will be converted to single precsion. This issue won't exist for other values, but for "min", the value will be modified.

Ping.

[craig.topper]

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

316

Why did this line change?

[craig.topper]

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

494

I think to fix the "min" problem for double and half, you need a separate version of this function for fli.h and fli.d so that you can call getLoadFP16Imm and getLoadFP64Imm. To do that you need a different AsmOperandClass for each instruction.

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

26

This error message isn't accurate. You have a floating point constant. It just isn't a valid one.

[craig.topper]

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

192

I don't see a test for parsing fli.d ft1, min as input.

237

There's no test for parsing the minimum value as an FP value.

[joshua-arch1]

Ping.

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

494

I tried to use separate ASMOperandClass for fli.h and fli.d, but values apart from "min" went wrong. As far as I'm concerned, support for specific values of "min" needs much more extra work than expected. It may even affect other normal value which is equal for fli.s/fli.d/fli.h. That is why spec suggests using "min". I don't think there will be occasions when we need to use specific values of "min".

494

When we write an assembly with a floating-point constant, it will be recognized as single-precision by default. That is why how we address "min" and other values differ.

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

237

As far as I'm concerned, support for specific values of "min" needs much more extra work than expected. It may even affect other normal value which is equal for fli.s/fli.d/fli.h. That is why spec suggests using "min". I don't think there will be occasions when we need to use specific values of "min". When we write an assembly with a floating-point constant, it will be recognized as single-precision by default. That is why how we address "min" and other values differ.

[craig.topper]

LGTM

[zixuan-wu]

In D140460#4172085, @craig.topper wrote:

LGTM

@craig.topper Thank you very much for your review support. It had been a long journey.

[craig.topper]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

400

This hack to support the f16 minimum value causes this to miscompile.

; FIXME: This is the f16 minimum value. It should not be supported for f32.
define float @loadfpimm10() {
; CHECK-LABEL: loadfpimm10:
; CHECK:       # %bb.0:
; CHECK-NEXT:    fli.s fa0, min
; CHECK-NEXT:    ret
  ret float 0.00006103515625
}