Page MenuHomePhabricator

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

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
luismarques added inline comments.Nov 10 2020, 7:23 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3235

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
665

Is it sufficient to just check hasStdExtF here?

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

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
995

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

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

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
995

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

1250–1253

Comment needs updating.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1986–1991

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

2010–2015

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

2140–2143

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
2213–2214

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
1986–1991

This comment still applies.

2010–2015

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
2268–2276

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

3132–3135

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
520

Please also sort this one.

1126

Ditto.

2207–2211

Needs sorting too.

llvm/lib/Target/RISCV/RISCVISelLowering.h
63–68

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
525

This line needs to be indented further

1131

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
63–68

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?