This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Add missing handling for half precision
ClosedPublic

Authored by nemanjai on May 1 2020, 9:34 PM.

Details

Summary

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.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=45776

Diff Detail

Event Timeline

nemanjai created this revision.May 1 2020, 9:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2020, 9:34 PM
nemanjai marked an inline comment as done.May 1 2020, 9:38 PM
nemanjai added inline comments.
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.

nemanjai marked an inline comment as done.May 4 2020, 8:31 AM
nemanjai added inline comments.
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
}
amyk added a subscriber: amyk.May 12 2020, 9:58 AM
amyk added inline comments.
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)?

nemanjai marked an inline comment as done.May 20 2020, 8:18 AM
nemanjai added inline comments.
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.

stefanp accepted this revision as: stefanp.May 21 2020, 10:55 AM
stefanp added a subscriber: stefanp.

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?

This revision is now accepted and ready to land.May 21 2020, 10:55 AM
amyk accepted this revision.May 21 2020, 3:36 PM

Thanks for answering my question, I think this looks good to me.

nemanjai marked an inline comment as done.May 22 2020, 5:46 AM
nemanjai added inline comments.
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.

This revision was automatically updated to reflect the committed changes.

LGTM.

llvm/test/CodeGen/PowerPC/handle-f16-storage-type.ll
202

Ok, makes sense. I should probably start doing this too in that case.