Page MenuHomePhabricator

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

Authored by joshua-arch1 on Dec 20 2022, 11:33 PM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
craig.topper added inline comments.Jan 3 2023, 10:21 AM
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 added inline comments.Jan 3 2023, 10:26 AM
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.

joshua-arch1 marked 11 inline comments as done.

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 added inline comments.Jan 12 2023, 9:31 PM
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.

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.

This change is missing a few required bits:

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 marked an inline comment as done.Jan 15 2023, 6:39 PM

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 added a comment.EditedJan 15 2023, 8:31 PM

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 added inline comments.Jan 17 2023, 8:52 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
164

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

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.

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 edited the summary of this revision. (Show Details)Jan 31 2023, 12:37 AM
joshua-arch1 edited the summary of this revision. (Show Details)Jan 31 2023, 9:54 PM

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 added a comment.Feb 7 2023, 2:40 AM

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 retitled this revision from [RISCV][MC] Add support for experimental zfa extension to [RISCV][MC] Add FLI instruction support for the experimental zfa extension.Feb 7 2023, 3:40 AM
joshua-arch1 added a comment.EditedFeb 8 2023, 7:00 PM

Ping.

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

craig.topper added inline comments.Feb 8 2023, 9:07 PM
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 added inline comments.Feb 10 2023, 10:13 PM
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 added inline comments.Feb 13 2023, 7:57 PM
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 marked 5 inline comments as done.Feb 15 2023, 10:52 PM

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 added inline comments.Feb 15 2023, 10:57 PM
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 added a comment.EditedFeb 19 2023, 11:15 PM

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

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.

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 added inline comments.Feb 21 2023, 12:03 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
316

Why did this line change?

craig.topper added inline comments.Feb 21 2023, 12:09 AM
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 added inline comments.Mar 5 2023, 8:35 PM
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.

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.

This revision is now accepted and ready to land.Mar 6 2023, 9:30 AM

LGTM

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

This revision was landed with ongoing or failed builds.Mar 6 2023, 10:06 PM
This revision was automatically updated to reflect the committed changes.
craig.topper added inline comments.Mar 7 2023, 7:52 PM
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
}