I recently fixed PR39865 but in doing so, missed a bunch of other cases that cause crashes in the PPC back end.
This patch handles those missed cases.
Details
- Reviewers
hfinkel jonpa uweigand stefanp amyk - Group Reviewers
Restricted Project - Commits
- rG1abba52044dd: [PowerPC] Add missing handling for half precision
rG1a493b0fa556: [PowerPC] Add missing handling for half precision
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/PowerPC/PPCISelLowering.h | ||
---|---|---|
643 | @uweigand This is missing from the SystemZ back end as well and it will cause a crash on vector-enabled subtargets with test/CodeGen/PowerPC/handle-f16-storage-type.ll when the legalizer tries to split an FP_EXTEND on a v1i16. The change doesn't really belong in this patch so I thought I'd just point it out. |
This is missing from the SystemZ back end
I don't think this change is right for SystemZ. This would change the ABI for single-element vector types. According to our ABI we want to pass those in vector registers, which requires widening to the full-width vector type. With your change, common code would instead scalarize to the scalar type, which means the parameter would be passed in GPRs instead.
llvm/lib/Target/PowerPC/PPCISelLowering.h | ||
---|---|---|
643 | Oh, I didn't realize this was part of the ABI. At some point, we might need to teach the legalizer to handle fp_round when legalizing by splitting. It currently crashes in such situations with define void @test_trunc32_vec4(<4 x float> %a, <4 x half>* %p) { %v = fptrunc <4 x float> %a to <4 x half> store <4 x half> %v, <4 x half>* %p ret void } |
llvm/test/CodeGen/PowerPC/scalar_vector_test_2.ll | ||
---|---|---|
11 | This test case seems to originally test for instructions like lfiwzx like the name of the function suggests, but that doesn't seem to be the case anymore. I am just wondering if this is a concern and should test purpose should change (in terms of maybe the function/file name, or something)? |
llvm/test/CodeGen/PowerPC/scalar_vector_test_2.ll | ||
---|---|---|
11 | I think we can probably remove this test case since a one element vector should be scalarized. |
One minor nit. Otherwise LGTM.
llvm/test/CodeGen/PowerPC/handle-f16-storage-type.ll | ||
---|---|---|
202 | All of these functions are marked as nounwind (due to the #0) do they need to be marked as such? |
llvm/test/CodeGen/PowerPC/handle-f16-storage-type.ll | ||
---|---|---|
202 | In general, when using the script to generate the checks, I prefer to use nounwind so that we don't get all the cfi directives that don't really contribute to the test - just add clutter. |
LGTM.
llvm/test/CodeGen/PowerPC/handle-f16-storage-type.ll | ||
---|---|---|
202 | Ok, makes sense. I should probably start doing this too in that case. |
@uweigand This is missing from the SystemZ back end as well and it will cause a crash on vector-enabled subtargets with test/CodeGen/PowerPC/handle-f16-storage-type.ll when the legalizer tries to split an FP_EXTEND on a v1i16.
The change doesn't really belong in this patch so I thought I'd just point it out.