This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add Zvfhmin extension support for llvm RISCV backend
ClosedPublic

Authored by jacquesguan on May 25 2023, 2:18 AM.

Diff Detail

Event Timeline

jacquesguan created this revision.May 25 2023, 2:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2023, 2:18 AM
jacquesguan requested review of this revision.May 25 2023, 2:18 AM
craig.topper added inline comments.May 31 2023, 1:09 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
184

Isn't the clang patch have dependent on this?

craig.topper added inline comments.May 31 2023, 1:19 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
184

I ran the zvfhmin.c from the clang patch through CodeGen and got

fatal error: error in backend: Scalarization of scalable vectors is not supported.

The clang patch should be reverted until the CodeGen patch is in.

craig.topper added inline comments.May 31 2023, 1:22 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1017

This needs to be after By default everything must be expanded.

Address comment and turn over dependency.

jacquesguan marked an inline comment as done.Jun 1 2023, 12:27 AM
jacquesguan added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
184

Yes, I made a mistake on the dependent relationship. Thanks for reverting it.

1017

Done.

eopXD added inline comments.Jun 1 2023, 10:51 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
6688

Nit: My mind is thinking of the sentence "this variable collects widen-able floating-point vectors that is widened from f16". I think the naming here didn't help me understand the code and I have actually read the RHS to understand the code here. Maybe we can improve this naming a nit.

jacquesguan marked an inline comment as done.Jun 1 2023, 7:51 PM
jacquesguan added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
6688

Yes, and any suggestion about naming?

craig.topper added inline comments.Jun 13 2023, 1:55 PM
llvm/test/CodeGen/RISCV/rvv/vfadd.ll
15

Is there a plan to support this via promotion in a future patch?

llvm/lib/Target/RISCV/RISCVFeatures.td
482

nit: Mininal -> Minimal

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

Need an isTypeLegal check?

From the v-spec:

When LMUL < SEW/ELEN, there is no guarantee an implementation would have enough bits in the fractional vector register to store at least one element, as VLEN=ELEN is a valid implementation choice. For example, with VLEN=ELEN=32, and SEWMIN=8, an LMUL of 1/8 would only provide four bits of storage in a vector register.

Consider the scenario when VT is <vscale x 1 x 16>. Then LMUL=MF4. If VLEN=ELEN=32 then LMUL=MF4 would only provide 8 bits of storage in a vector register when 16 is needed.

llvm/lib/Target/RISCV/RISCVSubtarget.h
165

nit: Mininal -> Minimal

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

Need to make other fp arithmetic operations not available? We're just relying on isel to not select the instructions, but should we should not give ISel the freedom to do this.

Address comment.

jacquesguan marked 3 inline comments as done.Jun 14 2023, 2:33 AM
jacquesguan added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
856

Done, thanks.

llvm/test/CodeGen/RISCV/rvv/vfadd.ll
15

Yes, I am working on a patch to do this.

craig.topper added inline comments.Jun 27 2023, 10:09 AM
llvm/lib/Support/RISCVISAInfo.cpp
155 ↗(On Diff #531237)

This needs to be rebased, this list has been alphabetized.

jacquesguan marked an inline comment as done.

rebase main

If I add RUN line with zvfhmin instead of zvfh llvm/test/CodeGen/RISCV/rvv/vfwnmacc-vp.ll, the compiler gives LLVM ERROR: Cannot select: t31: nxv1f32 = RISCVISD::VFWNMADD_VL t2, t4, t6, t8, t13, followed by trace. Probably need to check for zvfh in performVFMADD_VLCombine? Also maybe need to check in other functions like performFADDSUB_VLCombine?

michaelmaitland added inline comments.Aug 1 2023, 4:15 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
860

What is the reason not to use SetCommonVFPActions and SetCommonVFPExtLoadTruncStoreActions? I think we should be able to do all of these common VFP actions if we promote to f32 first.

jacquesguan added inline comments.Aug 1 2023, 6:58 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
860

Here is my patch for promoting f16 vector op zvfhmin https://reviews.llvm.org/D153848. As you say we can do all of these common VFP actions if we promote to f32 first, but then we have a f32 vector op and the action of it should be set under the f32 vector op scope which already used SetCommonVFPActions and SetCommonVFPExtLoadTruncStoreActions.

Address comment.

If I add RUN line with zvfhmin instead of zvfh llvm/test/CodeGen/RISCV/rvv/vfwnmacc-vp.ll, the compiler gives LLVM ERROR: Cannot select: t31: nxv1f32 = RISCVISD::VFWNMADD_VL t2, t4, t6, t8, t13, followed by trace. Probably need to check for zvfh in performVFMADD_VLCombine? Also maybe need to check in other functions like performFADDSUB_VLCombine?

Thanks to point it. I add a check to prevent the combination to widen op when we have only zvfhmin. But these test can not pass with RUN line zvfhmin because the vf form need promotion of splat, when implement in https://reviews.llvm.org/D153848.

If I add RUN line with zvfhmin instead of zvfh llvm/test/CodeGen/RISCV/rvv/vfwnmacc-vp.ll, the compiler gives LLVM ERROR: Cannot select: t31: nxv1f32 = RISCVISD::VFWNMADD_VL t2, t4, t6, t8, t13, followed by trace. Probably need to check for zvfh in performVFMADD_VLCombine? Also maybe need to check in other functions like performFADDSUB_VLCombine?

Thanks to point it. I add a check to prevent the combination to widen op when we have only zvfhmin. But these test can not pass with RUN line zvfhmin because the vf form need promotion of splat, when implement in https://reviews.llvm.org/D153848.

Should this patch depend on the type promotion patch then?

If I add RUN line with zvfhmin instead of zvfh llvm/test/CodeGen/RISCV/rvv/vfwnmacc-vp.ll, the compiler gives LLVM ERROR: Cannot select: t31: nxv1f32 = RISCVISD::VFWNMADD_VL t2, t4, t6, t8, t13, followed by trace. Probably need to check for zvfh in performVFMADD_VLCombine? Also maybe need to check in other functions like performFADDSUB_VLCombine?

Thanks to point it. I add a check to prevent the combination to widen op when we have only zvfhmin. But these test can not pass with RUN line zvfhmin because the vf form need promotion of splat, when implement in https://reviews.llvm.org/D153848.

Should this patch depend on the type promotion patch then?

No, actually the type promotion patch depends on this one. This patch added the basic support of zvfhmin which can only select to the several supported type convert instruction. https://reviews.llvm.org/D153848 uses promotion to support other ops.

I think that this patch and https://reviews.llvm.org/D153848 need to be committed at the same time. This will ensure that if the zvfhmin option is enabled, then we can codegen all LLVM IR.

I applied this patch and https://reviews.llvm.org/D153848, and then updated all instances of +zvfh to be +zvfhmin and re-ran the update_llc_test script for all of the files that were impacted. I found the following crash:

llvm/utils/update_llc_test_checks.py --llc-binary=build/bin/llc llvm/test/CodeGen/RISCV/regalloc-last-chance-recoloring-failure.ll
LLVM ERROR: Cannot select: intrinsic %llvm.riscv.vfwsub.w
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: build/bin/llc -mtriple=riscv64 -mattr=+f,+m,+zfh,+zvfhmin -riscv-enable-subreg-liveness=false
1.      Running pass 'Function Pass Manager' on module '<stdin>'.
2.      Running pass 'RISC-V DAG->DAG Pattern Instruction Selection' on function '@last_chance_recoloring_failure'
 #0 0x000000000178f197 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (build/bin/llc+0x178f197)
 #1 0x000000000178cebe llvm::sys::RunSignalHandlers() (build/bin/llc+0x178cebe)
 #2 0x000000000178f9af SignalHandler(int) Signals.cpp:0:0
 #3 0x00007f612c0b6cf0 __restore_rt (/lib64/libpthread.so.0+0x12cf0)
 #4 0x00007f612ab2aaff raise (/lib64/libc.so.6+0x4eaff)
 #5 0x00007f612aafdea5 abort (/lib64/libc.so.6+0x21ea5)
 #6 0x000000000170cad3 llvm::report_fatal_error(llvm::Twine const&, bool) (build/bin/llc+0x170cad3)
 #7 0x00000000015b1e3d llvm::SelectionDAGISel::CannotYetSelect(llvm::SDNode*) (build/bin/llc+0x15b1e3d)
 #8 0x00000000015b0cfb llvm::SelectionDAGISel::SelectCodeCommon(llvm::SDNode*, unsigned char const*, unsigned int) (build/bin/llc+0x15b0cfb)
 #9 0x000000000072465f llvm::RISCVDAGToDAGISel::Select(llvm::SDNode*) (build/bin/llc+0x72465f)
#10 0x00000000015a60f5 llvm::SelectionDAGISel::DoInstructionSelection() (build/bin/llc+0x15a60f5)
#11 0x00000000015a4fe2 llvm::SelectionDAGISel::CodeGenAndEmitDAG() (build/bin/llc+0x15a4fe2)
#12 0x00000000015a2b71 llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) (build/bin/llc+0x15a2b71)
#13 0x000000000159f69c llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) (build/bin/llc+0x159f69c)
#14 0x0000000000be8e2c llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (build/bin/llc+0xbe8e2c)
#15 0x0000000001102e23 llvm::FPPassManager::runOnFunction(llvm::Function&) (build/bin/llc+0x1102e23)
#16 0x000000000110aaa1 llvm::FPPassManager::runOnModule(llvm::Module&) (build/bin/llc+0x110aaa1)
#17 0x0000000001103626 llvm::legacy::PassManagerImpl::run(llvm::Module&) (build/bin/llc+0x1103626)
#18 0x00000000006987e8 compileModule(char**, llvm::LLVMContext&) llc.cpp:0:0
#19 0x00000000006962bd main (build/bin/llc+0x6962bd)
#20 0x00007f612ab16d85 __libc_start_main (/lib64/libc.so.6+0x3ad85)
#21 0x0000000000692c6e _start (build/bin/llc+0x692c6e)

I think we need to be able to update this patch or the type promotion patch to make sure we don't get any crashes like this when we run the experiment I describe above.

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

Do we need this TODO still?

1037

Do we need this TODO still?

I think that this patch and https://reviews.llvm.org/D153848 need to be committed at the same time. This will ensure that if the zvfhmin option is enabled, then we can codegen all LLVM IR.

I applied this patch and https://reviews.llvm.org/D153848, and then updated all instances of +zvfh to be +zvfhmin and re-ran the update_llc_test script for all of the files that were impacted. I found the following crash:

llvm/utils/update_llc_test_checks.py --llc-binary=build/bin/llc llvm/test/CodeGen/RISCV/regalloc-last-chance-recoloring-failure.ll
LLVM ERROR: Cannot select: intrinsic %llvm.riscv.vfwsub.w
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: build/bin/llc -mtriple=riscv64 -mattr=+f,+m,+zfh,+zvfhmin -riscv-enable-subreg-liveness=false
1.      Running pass 'Function Pass Manager' on module '<stdin>'.
2.      Running pass 'RISC-V DAG->DAG Pattern Instruction Selection' on function '@last_chance_recoloring_failure'
 #0 0x000000000178f197 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (build/bin/llc+0x178f197)
 #1 0x000000000178cebe llvm::sys::RunSignalHandlers() (build/bin/llc+0x178cebe)
 #2 0x000000000178f9af SignalHandler(int) Signals.cpp:0:0
 #3 0x00007f612c0b6cf0 __restore_rt (/lib64/libpthread.so.0+0x12cf0)
 #4 0x00007f612ab2aaff raise (/lib64/libc.so.6+0x4eaff)
 #5 0x00007f612aafdea5 abort (/lib64/libc.so.6+0x21ea5)
 #6 0x000000000170cad3 llvm::report_fatal_error(llvm::Twine const&, bool) (build/bin/llc+0x170cad3)
 #7 0x00000000015b1e3d llvm::SelectionDAGISel::CannotYetSelect(llvm::SDNode*) (build/bin/llc+0x15b1e3d)
 #8 0x00000000015b0cfb llvm::SelectionDAGISel::SelectCodeCommon(llvm::SDNode*, unsigned char const*, unsigned int) (build/bin/llc+0x15b0cfb)
 #9 0x000000000072465f llvm::RISCVDAGToDAGISel::Select(llvm::SDNode*) (build/bin/llc+0x72465f)
#10 0x00000000015a60f5 llvm::SelectionDAGISel::DoInstructionSelection() (build/bin/llc+0x15a60f5)
#11 0x00000000015a4fe2 llvm::SelectionDAGISel::CodeGenAndEmitDAG() (build/bin/llc+0x15a4fe2)
#12 0x00000000015a2b71 llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) (build/bin/llc+0x15a2b71)
#13 0x000000000159f69c llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) (build/bin/llc+0x159f69c)
#14 0x0000000000be8e2c llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (build/bin/llc+0xbe8e2c)
#15 0x0000000001102e23 llvm::FPPassManager::runOnFunction(llvm::Function&) (build/bin/llc+0x1102e23)
#16 0x000000000110aaa1 llvm::FPPassManager::runOnModule(llvm::Module&) (build/bin/llc+0x110aaa1)
#17 0x0000000001103626 llvm::legacy::PassManagerImpl::run(llvm::Module&) (build/bin/llc+0x1103626)
#18 0x00000000006987e8 compileModule(char**, llvm::LLVMContext&) llc.cpp:0:0
#19 0x00000000006962bd main (build/bin/llc+0x6962bd)
#20 0x00007f612ab16d85 __libc_start_main (/lib64/libc.so.6+0x3ad85)
#21 0x0000000000692c6e _start (build/bin/llc+0x692c6e)

I think we need to be able to update this patch or the type promotion patch to make sure we don't get any crashes like this when we run the experiment I describe above.

Yes, I agree with merging these patches at the same time.

But for the case regalloc-last-chance-recoloring-failure.ll, it uses the intirinsic @llvm.riscv.vfwsub.w.nxv16f32.nxv16f16.i64, this intrinsic is not supported in zvfhmin, so cannot selected is the excepted action. By example we also cannot select @llvm.riscv.vmulh.nxv1i64.i64 for ZVE64D, and any vector intrinsic for no V enable. I don't think we should do promotion for intrinsic too.
Actually target specific intrinsics basiclly comes from clang rvv-builtin. In https://reviews.llvm.org/D150253, any use of unsuportted builtin would cause a clang error. So there is no actual case will generate unsupportted intrinsic.

rebase main

Yes, I agree with merging these patches at the same time.

But for the case regalloc-last-chance-recoloring-failure.ll, it uses the intirinsic @llvm.riscv.vfwsub.w.nxv16f32.nxv16f16.i64, this intrinsic is not supported in zvfhmin, so cannot selected is the excepted action. By example we also cannot select @llvm.riscv.vmulh.nxv1i64.i64 for ZVE64D, and any vector intrinsic for no V enable. I don't think we should do promotion for intrinsic too.
Actually target specific intrinsics basiclly comes from clang rvv-builtin. In https://reviews.llvm.org/D150253, any use of unsuportted builtin would cause a clang error. So there is no actual case will generate unsupportted intrinsic.

Thanks for the clarification. I agree with you now.

michaelmaitland accepted this revision.Aug 14 2023, 8:28 PM

LGTM. Please wait to merge this and the type promotion patch together.

This revision is now accepted and ready to land.Aug 14 2023, 8:28 PM

Rebase main.

jacquesguan retitled this revision from [RISCV] Add Zvfhmin extension support for llvm RISCV backend. to [RISCV] Add Zvfhmin extension support for llvm RISCV backend.Aug 22 2023, 7:40 PM