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

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?

Does this need to accept rv32izfh for -march option

Address @luismarques' comments.

Harbormaster completed remote builds in B78441: Diff 304476.Nov 11 2020, 5:22 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

In D90738#2387525, @kito-cheng wrote:

Could you add zfh to ELF attribute output?

https://reviews.llvm.org/D91314

Remove unnecessary checking for StdExtZfh and StdExtD.

Harbormaster completed remote builds in B78552: Diff 304699.Nov 11 2020, 7:05 PM

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?

dexonsmith removed a subscriber: dexonsmith.Nov 17 2020, 11:01 AM

Address @craig.topper's comments.

Use MCRegister in RISCVAsmParser.cpp.

Harbormaster completed remote builds in B79366: Diff 306226.Nov 18 2020, 2:45 PM

Harbormaster completed remote builds in B79367: Diff 306228.Nov 18 2020, 2:49 PM

LGTM

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)?

In D90738#2411538, @luismarques wrote:

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.

In D90738#2411702, @jrtc27 wrote:

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.

Update test cases.

Harbormaster completed remote builds in B79908: Diff 307280.Nov 24 2020, 1:34 AM

HsiangKai updated this revision to Diff 307282.Nov 24 2020, 1:42 AM

Harbormaster completed remote builds in B79910: Diff 307282.Nov 24 2020, 1:43 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

Harbormaster completed remote builds in B79939: Diff 307328.Nov 24 2020, 6:16 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

Harbormaster completed remote builds in B80084: Diff 307584.Nov 25 2020, 5:26 AM

LGTM

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

Address @jrtc27's comments.

HsiangKai updated this revision to Diff 308196.Nov 28 2020, 7:24 PM

Harbormaster completed remote builds in B80440: Diff 308196.Nov 28 2020, 7:42 PM

Harbormaster completed remote builds in B80439: Diff 308195.

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

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

LGTM

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.

Harbormaster completed remote builds in B80621: Diff 308557.Dec 1 2020, 1:24 AM

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

Harbormaster completed remote builds in B80753: Diff 308839.Dec 1 2020, 7:44 PM

Harbormaster completed remote builds in B80756: Diff 308846.Dec 1 2020, 7:53 PM

Harbormaster completed remote builds in B80758: Diff 308850.Dec 1 2020, 7:58 PM

LGTM. @jrtc27 does this look ok to you?

Yes that looks like everything now; thanks!

This revision was landed with ongoing or failed builds.Dec 2 2020, 5:17 PM

Closed by commit rGf7bc7c2981d4: [RISCV] Support Zfh half-precision floating-point extension. (authored by HsiangKai). · Explain Why

This revision was automatically updated to reflect the committed changes.

HsiangKai added a commit: rGf7bc7c2981d4: [RISCV] Support Zfh half-precision floating-point extension..

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

Herald added subscribers: StephenFan, vkmr. · View Herald TranscriptMar 4 2021, 7:15 AM

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 309110

llvm/include/llvm/IR/RuntimeLibcalls.def

llvm/lib/CodeGen/TargetLoweringBase.cpp

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

llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp

llvm/lib/Target/RISCV/RISCV.td

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

llvm/lib/Target/RISCV/RISCVISelLowering.h

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

llvm/lib/Target/RISCV/RISCVInstrInfo.td

llvm/lib/Target/RISCV/RISCVInstrInfoZfh.td

llvm/lib/Target/RISCV/RISCVMergeBaseOffset.cpp

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

llvm/lib/Target/RISCV/RISCVRegisterInfo.td

llvm/lib/Target/RISCV/RISCVSubtarget.h

llvm/test/CodeGen/RISCV/copysign-casts.ll

llvm/test/CodeGen/RISCV/half-arith.ll

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

llvm/test/CodeGen/RISCV/half-br-fcmp.ll

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

llvm/test/CodeGen/RISCV/half-fcmp.ll

llvm/test/CodeGen/RISCV/half-imm.ll

llvm/test/CodeGen/RISCV/half-intrinsics.ll

llvm/test/CodeGen/RISCV/half-isnan.ll

llvm/test/CodeGen/RISCV/half-mem.ll

llvm/test/CodeGen/RISCV/half-select-fcmp.ll

llvm/test/CodeGen/RISCV/rv32i-rv64i-half.ll

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

llvm/test/CodeGen/RISCV/zfh-imm.ll

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

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

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

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

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

llvm/test/MC/RISCV/rvzfh-aliases-valid.s

llvm/test/MC/RISCV/rvzfh-pseudos.s

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

Revision Contents
Changeset List