This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support Zfh half-precision floating-point extension.
ClosedPublic

Authored by HsiangKai on Nov 3 2020, 10:44 PM.

Diff Detail

Event Timeline

HsiangKai created this revision.Nov 3 2020, 10:44 PM
HsiangKai requested review of this revision.Nov 3 2020, 10:44 PM
craig.topper added inline comments.Nov 4 2020, 12:04 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1579

Can we line break this the same way as FPR32s/FPR64s for consisency?

1760

The second argument is shadow regs. Do we need a way to shaddow ArgFPR16s here.

1764

Does this also need to shadow ArgFPR64s?

2885

It looks like this code was trying to pick the largest register size. Shouldn't we keep the _F register if StdExtD is disabled regardless of whether Zfh is enabled?

craig.topper added inline comments.Nov 4 2020, 3:31 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1760

Thinking more about this, since the fp registers are sub/super registers, we may not need shadows registers here at all.

I've posted D90801 to remove the shadow arguments here.

1927

Drop the else. The break in the previous if already exited.

The other option would be to remove breaks and pull the BITCAST below into a final else.

llvm/lib/Target/RISCV/RISCVInstrInfoZfh.td
293

I think its possible to have an fcopysign with a FPR32 or FPR64 second argument. It's also possible to have a fcopysign with a FPR32 or FPR64 first argument and an FPR16 second argument.

RISCVInstrInfoD.td has these patterns because of that

def : Pat<(fcopysign FPR64:$rs1, FPR32:$rs2), (FSGNJ_D $rs1, (FCVT_D_S $rs2))>;                                                                                                                                                                                                    
def : Pat<(fcopysign FPR32:$rs1, FPR64:$rs2), (FSGNJ_S $rs1, (FCVT_S_D $rs2,                                                                                                                                                                                                       
                                                              0b111))>;

Do we need custom handling for i16<->f16 bitcasts in ReplaceNodeResults and LowerOperation?

HsiangKai updated this revision to Diff 303011.Nov 4 2020, 7:00 PM

Address @craig.topper's comments. Deal with i16 <-> f16 later.

HsiangKai added inline comments.Nov 4 2020, 7:01 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1579

It is arranged by clang-format. What should I follow? I also prefer to keep the consistency.

1760

I agree with you. Thanks.

craig.topper added inline comments.Nov 4 2020, 7:19 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1579

I find the FPR32s/FPR64s easier to read so I think we can ignore clang-format here.

2886

I don't think this is needed at all. Won't StdExtF be enabled when Zfh is enabled?

HsiangKai updated this revision to Diff 303041.Nov 4 2020, 10:56 PM
  • Reformatting.
  • Remove unnecessary code.
craig.topper added inline comments.Nov 5 2020, 3:29 PM
llvm/lib/Target/RISCV/RISCVInstrInfoZfh.td
274

I don't think these bitconvert patterns make sense. bitconvert must have the same number of bits on both sides. The GPR will be i32 or i64 which will never have the same number of bit as f16.

HsiangKai updated this revision to Diff 303338.Nov 5 2020, 11:16 PM
  • Remove useless bitconvert patterns.
  • Support f16 <-> i16 conversions.
  • Add test cases for f16 <-> i16 conversions.
HsiangKai updated this revision to Diff 303339.Nov 5 2020, 11:23 PM
  • Rename test cases.
xmj added a subscriber: xmj.Nov 8 2020, 7:09 PM

This patch is pretty big, so I haven't yet been able to delve as deeply as I wanted to, but so far I didn't notice any major issues.
Thanks to Craig for providing the initial round of feedback, and pointing out several issues.
Some nitpicking remarks:

  • Several tests should probably use a hard-float ABI, to cut down on the fmvs.
  • For files that handle F, D and half, it would be nice to try to be more consistent regarding the order in which the code and declarations for those appears. (When handling early returns, short-circuit evaluation, and so on, it might arguably still make sense to be inconsistent and order things based on expected frequency, as that might have a very slightly compile-time performance impact)
llvm/lib/Target/RISCV/RISCVISelLowering.h
61–64

Nit: move this to before the READ_CYCLE_WIDE, so that it's logically closer to the other move nodes. Tweak the comment to generalize it, etc.

luismarques added inline comments.Nov 10 2020, 7:23 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2844

Presumably we don't need the test for hasStdExtZfh, since hasStdExtF would match anyway.

llvm/test/CodeGen/RISCV/copysign-casts.ll
12–17

If we have both RV32IF and RV32IFD, I wonder if we should also have RV32IFZfh, instead of only RV32IFDZfh.

llvm/test/CodeGen/RISCV/half-arith.ll
3–6

This test should probably use a hard-float ABI, to cut down on the fmvs.

Could you add zfh to ELF attribute output?

Jim added a comment.Nov 10 2020, 9:13 PM

Does this need to accept rv32izfh for -march option

HsiangKai updated this revision to Diff 304476.Nov 11 2020, 4:17 AM
craig.topper added inline comments.Nov 11 2020, 12:40 PM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
655

Is it sufficient to just check hasStdExtF here?

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2844

Checking D here is also unnecessary isn't it?

In D90738#2387689, @Jim wrote:

Does this need to accept rv32izfh for -march option

https://reviews.llvm.org/D91315

Could you add zfh to ELF attribute output?

https://reviews.llvm.org/D91314

HsiangKai updated this revision to Diff 304699.Nov 11 2020, 6:25 PM
  • Remove unnecessary checking for StdExtZfh and StdExtD.
asb added a comment.Nov 12 2020, 5:37 AM

Historically for the RISC-V backend we've split patches into the MC layer changes and the codegen changes in order to make them easier to review. Do you think you'd be able to do the same here?

In D90738#2391104, @asb wrote:

Historically for the RISC-V backend we've split patches into the MC layer changes and the codegen changes in order to make them easier to review. Do you think you'd be able to do the same here?

Zfh extension is similar to F and D extension. I use F and D extension as the reference to implement Zfh. So, I implement the feature in one patch. I have no separate patches for MC and codegen on my hands.

craig.topper added inline comments.Nov 16 2020, 3:29 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
992

This needs to be rebased. This file uses MCRegister instead of Register now.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2738

This needs to be rebased to use NODE_NAME_CASE macros.

llvm/test/CodeGen/RISCV/half-bitmanip-dagcombines.ll
111

"halfing point" here looks like a bad search and replace of "float". Can you change to "half precision floating point" and check if this occurs anywhere else.

llvm/test/CodeGen/RISCV/half-convert.ll
476

Are we missing coverage of conversions to/from double with D extension enabled?

HsiangKai updated this revision to Diff 306226.Nov 18 2020, 2:34 PM

Address @craig.topper's comments.

HsiangKai updated this revision to Diff 306228.Nov 18 2020, 2:38 PM

Use MCRegister in RISCVAsmParser.cpp.

This revision is now accepted and ready to land.Nov 18 2020, 7:21 PM

I had added an inline comment for half-arith.ll nitpicking that it should probably use a hard-float ABI, to cut down on the fmvs. Now it has test checks for both soft float and hard float ABIs. @HsiangKai Is this because you think there is value in testing both scenarios in that file (and several others)?

I had added an inline comment for half-arith.ll nitpicking that it should probably use a hard-float ABI, to cut down on the fmvs. Now it has test checks for both soft float and hard float ABIs. @HsiangKai Is this because you think there is value in testing both scenarios in that file (and several others)?

I didn't think about it too much. I just enumerate the possible combinations of abi and zfh.

My intuition is that there should be tests for the various instructions that are only hard-float ABIs, and tests specifically for all the different calling conventions that use a very limited set of instructions.

My intuition is that there should be tests for the various instructions that are only hard-float ABIs, and tests specifically for all the different calling conventions that use a very limited set of instructions.

Agree. I will refine these test cases.

HsiangKai updated this revision to Diff 307280.Nov 24 2020, 1:34 AM

Update test cases.

HsiangKai updated this revision to Diff 307282.Nov 24 2020, 1:42 AM
jrtc27 added inline comments.Nov 24 2020, 2:06 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
992

Nit: I'd put FPR16 before FPR32 in this file (here and below).

1253–1254

Comment needs updating.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1633–1638

These two are always the same value (unless you want an RV16I!), just rename the F32 one to encompass F16.

1662–1667

Hm, this code is a bit silly, we only need to look at one of the arrays as the registers all alias each other.

1794–1797

Since this code is not already ISA/ABI-dependent this should be fine?

llvm/lib/Target/RISCV/RISCVRegisterInfo.td
33–35

Needs updating to mention RISCVReg16? Though this is currently backwards; the 64-bit register is the canonical one given the diff in RISCVAsmParser...

Overall this still looks good to me. Could you please just fix the following inconsistency?

; RUN: llc -mtriple=riscv32 -mattr=+experimental-zfh -verify-machineinstrs \
; RUN:   -target-abi ilp32f < %s | FileCheck -check-prefix=RV32IZFH-ILP32F %s

; RUN: llc -mtriple=riscv32 -mattr=+experimental-zfh -verify-machineinstrs \
; RUN:   -target-abi ilp32f < %s | FileCheck -check-prefix=RV32IF-ILP32F %s

Besides being inconsistent, the latter just seems wrong to me.

HsiangKai updated this revision to Diff 307328.Nov 24 2020, 6:12 AM
craig.topper added inline comments.Nov 24 2020, 6:44 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1871

If you're going to make changes to this patch. Can you move this into a final else and pull out the "break;" Similar to the code in convertValVTToLocVT. Sorry I missed it earlier.

HsiangKai updated this revision to Diff 307584.Nov 25 2020, 4:44 AM
jrtc27 requested changes to this revision.Nov 25 2020, 4:07 PM
jrtc27 added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1633–1638

This comment still applies.

1662–1667

Ditto

This revision now requires changes to proceed.Nov 25 2020, 4:07 PM
HsiangKai updated this revision to Diff 308195.Nov 28 2020, 7:17 PM

Address @jrtc27's comments.

HsiangKai updated this revision to Diff 308196.Nov 28 2020, 7:24 PM
Harbormaster completed remote builds in B80439: Diff 308195.

This looks good to me. But I'll let @jrtc27 approve

jrtc27 accepted this revision.Nov 30 2020, 9:45 AM

Couple of minor changes but otherwise LGTM.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1925–1933

Sorry for continuing to nitpick (and not spotting this first time) but could you please sort these (probably by ValVT then LocVT is best)?

2704–2707

In a similar vein, put these before the W versions?

This revision is now accepted and ready to land.Nov 30 2020, 9:45 AM
HsiangKai updated this revision to Diff 308557.Dec 1 2020, 12:25 AM

I thought I'd gone through it thoroughly last time but apparently not :( oh well hopefully the last set of nits.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
495

Please also sort this one.

1071

Ditto.

1861

Needs sorting too.

llvm/lib/Target/RISCV/RISCVISelLowering.h
45–52

Comment should be updated to reflect the sorted order. It's also unclear which combinations are invalid; currently it could be interpreted as f16 only being an issue on RV32.

HsiangKai updated this revision to Diff 308839.Dec 1 2020, 6:59 PM
craig.topper added inline comments.Dec 1 2020, 7:02 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
500

This line needs to be indented further

1076

Same here

HsiangKai updated this revision to Diff 308846.Dec 1 2020, 7:24 PM
jrtc27 added inline comments.Dec 1 2020, 7:28 PM
llvm/lib/Target/RISCV/RISCVISelLowering.h
45–52

Still not clear that:
(a) i16<->f16 is a problem everywhere
(b) i32<->f32 is a problem only for RV64

Maybe it's best to generalise the opening paragraph to something like:

FPR<->GPR transfer operations when the FPR is smaller than XLEN, needed as XLEN is the only legal integer width.

HsiangKai updated this revision to Diff 308850.Dec 1 2020, 7:40 PM

LGTM. @jrtc27 does this look ok to you?

jrtc27 accepted this revision.Dec 2 2020, 10:05 AM

Yes that looks like everything now; thanks!

This revision was landed with ongoing or failed builds.Dec 2 2020, 5:17 PM
This revision was automatically updated to reflect the committed changes.

Is there a missing test for fcvt.h.d in the test case?