Support "Zfh" extension according to https://github.com/riscv/riscv-isa-manual/blob/zfh/src/zfh.tex.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
1784 | Can we line break this the same way as FPR32s/FPR64s for consisency? | |
1965 | The second argument is shadow regs. Do we need a way to shaddow ArgFPR16s here. | |
1969 | Does this also need to shadow ArgFPR64s? | |
3078 | 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? |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
1965 | 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. | |
2132 | 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?
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. |
- Remove useless bitconvert patterns.
- Support f16 <-> i16 conversions.
- Add test cases for f16 <-> i16 conversions.
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. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
3037 | 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. |
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.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
---|---|---|
990 | This needs to be rebased. This file uses MCRegister instead of Register now. | |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
2913 | 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? |
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.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
---|---|---|
990 | Nit: I'd put FPR16 before FPR32 in this file (here and below). | |
1251–1252 | Comment needs updating. | |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
1838–1843 | ||
1867–1872 | Hm, this code is a bit silly, we only need to look at one of the arrays as the registers all alias each other. | |
1999–2001 | 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.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
2076 | 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. |
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 | ||
---|---|---|
499 | Please also sort this one. | |
1077 | Ditto. | |
2066 | 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. |
llvm/lib/Target/RISCV/RISCVISelLowering.h | ||
---|---|---|
45–52 | Still not clear that: Maybe it's best to generalise the opening paragraph to something like:
|
This needs to be rebased. This file uses MCRegister instead of Register now.