This is an archive of the discontinued LLVM Phabricator instance.

[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

joshua-arch1 created this revision.Dec 20 2022, 11:33 PM
joshua-arch1 requested review of this revision.Dec 20 2022, 11:33 PM
joshua-arch1 edited the summary of this revision. (Show Details)Dec 20 2022, 11:38 PM
joshua-arch1 added a reviewer: kito-cheng.
craig.topper added inline comments.
llvm/lib/Target/RISCV/RISCV.td
132

This isn't used anywhere.

llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td
2

File name says Zfh

llvm/test/MC/RISCV/rv64zfa.s
82 ↗(On Diff #484475)

Add new line

craig.topper added inline comments.Dec 21 2022, 12:47 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
347

Can we use a smaller size than unsigned int?

355

Same here

llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td
89

These instructions need a Zfa assembler predicate

92

These instructions need a Zfa and D assembler predicate

95

These instructions need a Zfa and Zfh assembler predicate

joshua-arch1 edited the summary of this revision. (Show Details)Dec 21 2022, 12:49 AM
joshua-arch1 added a reviewer: craig.topper.

All of the instructions need predicates. I was only giving specific examples.

craig.topper added inline comments.Dec 21 2022, 10:05 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1639

what about "min"? mentioned in the spec?

Need a rv64zfa-invalid.s test to make sure we don't parse these instructions if the Zfa feature isn't enabled.

llvm/test/MC/RISCV/rv64zfa.s
1 ↗(On Diff #484494)

Aren't our assembler tests usually named something like rv64zfa-valid.s?

craig.topper added inline comments.Dec 21 2022, 10:17 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
347

Use uint8_t.

355

Use uint8_t

412

I can't find any caller that checks for the -1 value. Did I miss it?

joshua-arch1 edited the summary of this revision. (Show Details)Dec 21 2022, 10:43 PM
joshua-arch1 edited the summary of this revision. (Show Details)
joshua-arch1 edited the summary of this revision. (Show Details)Dec 21 2022, 11:17 PM
joshua-arch1 marked 7 inline comments as done.
joshua-arch1 edited the summary of this revision. (Show Details)Dec 22 2022, 10:53 PM
joshua-arch1 edited the summary of this revision. (Show Details)Dec 23 2022, 12:04 AM

I found this binutils patch that seems to treat integers without "0x" as indices for FLI. https://github.com/a4lg/binutils-gdb/pull/56/files

craig.topper added inline comments.Dec 30 2022, 9:15 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
385

All of the values are in order. Can we use a constexpr array of std::pair<uint8_t, uint8_t> and look it up with std::lower_bound? Once we have the iterator we can use std::distance+1 to convert to 1-31.

llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td
107

Extra space after Zfh

craig.topper added inline comments.Dec 30 2022, 9:20 AM
llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td
11

Extra '\' at the end of the line

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

I don't like the duplicated test content with rv32zfa-valid.s

I believe our usual MC testing is done like this

rv32zfa-valid.s <- test with both riscv32 and riscv64 command lines.
rv32zfa-only-valid.s <- test with riscv32 only instructions that aren't not supported by riscv64

joshua-arch1 marked 2 inline comments as done.Jan 3 2023, 12:15 AM

I found this binutils patch that seems to treat integers without "0x" as indices for FLI. https://github.com/a4lg/binutils-gdb/pull/56/files

I used hexadecimal representation with "0x" to distinguish the integer value 1/2/3/4/8/16 from the floatin-point value 1.0/2.0/3.0/4.0/8.0/16.0. To keep consistent with gcc, now I try to remove "0x" and directly use the decimal encoding. That seems to work too.

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

I'm afraid not. The values in the pair are not in order.

std::lower_bound returns an iterator pointing to the first element in the range [first, last) that is not less than (i.e. greater or equal to) value, or last if no such element is found.

We cannot choose a value that can help us get the index.

385

I use an array of std::pair<uint8_t, uint8_t> with std::find and std::distance to simplify this function. Thank you for your suggestion.

412

Sorry I did forget to use this "-1" again. What I'd like to do here is to distinguish between different floating-point immediates and check whether the value can be applied as the operand in fli.s/fli.d/fli.h instruction. I have added this check in isLoadFPImm() in RISCVAsmParser.cpp.

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

Thank you for your advice. I'll modify mt testing, but in some MC testing like Zfh and Zfhmin, rv32 and rv64 are tested in separate files.

craig.topper added inline comments.Jan 3 2023, 12:48 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
385

I thought I checked them which ones aren’t in order?

craig.topper added inline comments.Jan 3 2023, 10:21 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
400

You might be able to use llvm::find(LoadFPImmArr)... here.

craig.topper added inline comments.Jan 3 2023, 10:21 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1654

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

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

const

347

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

355

const

375

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

385

const

404

No need for else since the if body returned

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

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
2

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
166

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
1633

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
1633

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

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
166

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
166

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
166

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
1620

Capitalize isNegative

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

Use uint32_t for the result to make it explicitly 32 bits

389

"as an" -> "as a"

392

You can use Imm.extractBitsAsZExtValue to extract the fields.

406

Use extractBitsAsZExtValue

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

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

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
313

Is this IsRV64 field used ever?

llvm/test/MC/RISCV/zfa-valid.s
197 ↗(On Diff #496318)

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
407

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
313

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

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

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 ↗(On Diff #496318)

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
407

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
407

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

407

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.

407

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
326–327

Why did this line change?

craig.topper added inline comments.Feb 21 2023, 12:09 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
548

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 ↗(On Diff #497959)

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 ↗(On Diff #502505)

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

237 ↗(On Diff #502505)

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

Ping.

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

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

548

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 ↗(On Diff #502505)

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
392

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
}