This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Handle f16 as a storage type only
ClosedPublic

Authored by nemanjai on Sep 30 2019, 12:15 PM.

Details

Reviewers
hfinkel
jsji
Group Reviewers
Restricted Project
Commits
rG512600e3c0db: [PowerPC] Handle f16 as a storage type only
Summary

The PPC back end currently crashes (fails to select) with f16 input. This patch expands it on subtargets prior to ISA 3.0 (Power9) and uses the HW conversions on Power9.

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

Diff Detail

Event Timeline

nemanjai created this revision.Sep 30 2019, 12:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2019, 12:15 PM
shchenz added inline comments.Oct 10 2019, 7:44 PM
lib/Target/PowerPC/PPCISelLowering.cpp
184 ↗(On Diff #222470)

Do we need to handle ppcf128 also?

lib/Target/PowerPC/PPCInstrVSX.td
114 ↗(On Diff #222470)

Guard under IsISA3_0?

3263 ↗(On Diff #222470)

Guard under IsISA3_0?

test/CodeGen/PowerPC/handle-f16-storage-type.ll
8 ↗(On Diff #222470)

#0 , seems all the function attributes are not defined?

nemanjai marked 4 inline comments as done.Oct 11 2019, 5:28 AM
nemanjai added inline comments.
lib/Target/PowerPC/PPCISelLowering.cpp
184 ↗(On Diff #222470)

Not really. That type is really just a pair of doubles and there is no register that can contain it, so it will always be broken up into a pair of doubles by the legalizer.

lib/Target/PowerPC/PPCInstrVSX.td
114 ↗(On Diff #222470)

Why? This is just a def of a pattern fragment. It is only defined here because it is missing in the target independent td file.

3263 ↗(On Diff #222470)

The instructions used are defined in a Power9Vector guard and so are these patterns. I realize this is hard to track down from the patch - this file is in desperate need of refactoring :(

test/CodeGen/PowerPC/handle-f16-storage-type.ll
8 ↗(On Diff #222470)

Sure, I can get rid of these (actually, I'll just define #0 as nounwind and use it for all the functions so we don't get the CFI nodes).

lei added a subscriber: lei.Nov 12 2019, 7:40 AM
lei added inline comments.
lib/Target/PowerPC/PPCInstrVSX.td
3272 ↗(On Diff #222470)

Should we clear the side effect bit for these? let hasSideEffects = 0

Other than the test case which still needs to be updated I think that this patch looks good to me.

lib/Target/PowerPC/PPCInstrVSX.td
3272 ↗(On Diff #222470)

I do not believe that Pat has a Side Effect flag.

The instruction STXSIHX is already marked as let hasSideEffects = 0.
For XSCVDPHP we do want to have side effects because it alters special registers that are not modeled by the operands on the instruction.

nemanjai updated this revision to Diff 241445.Jan 30 2020, 7:02 AM

Updated the test case.

@lei @stefanp Could you please re-review as this would be good to fix since it fixes a reported bug? I would like to get it committed and backported to V10.0.

Unit tests: fail. 62343 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

nemanjai edited the summary of this revision. (Show Details)Feb 5 2020, 10:32 AM
nemanjai updated this revision to Diff 242698.Feb 5 2020, 10:57 AM

My good intentions of updating and uploading the test case were once again thwarted by git in the previous update. Actually included the test case on this one.

Unit tests: pass. 62515 tests passed, 0 failed and 844 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jsji added inline comments.Feb 5 2020, 2:14 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
170

Do we need to consider useSoftFloat here?

llvm/lib/Target/PowerPC/PPCInstrVSX.td
114

Why we need such PatFrag while the other targets with fp16 support not? Is it due to that we only want to handle storage types in this patch?

If this is really needed, why not move it to TargetSelectionDAG.td to be together with extloadf32 and extloadf64?

llvm/test/CodeGen/PowerPC/handle-f16-storage-type.ll
35 ↗(On Diff #242698)

Nit: Function Attrs deleted but comments still kept.

Mostly nits from me.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
170

It's a good question. I did run the test with soft float and it seemed to have worked correctly.
It appears that type legalization will get rid of all float types ahead of time because of this:

if (!useSoftFloat()) {
    if (hasSPE()) {
      addRegisterClass(MVT::f32, &PPC::GPRCRegClass);
      addRegisterClass(MVT::f64, &PPC::SPERCRegClass);
    } else {
      addRegisterClass(MVT::f32, &PPC::F4RCRegClass);
      addRegisterClass(MVT::f64, &PPC::F8RCRegClass);
    }
  }

So, I think we may be ok. However, I will wait for Nemanja to confirm that what I'm saying makes sense.

llvm/test/CodeGen/PowerPC/handle-f16-storage-type.ll
36 ↗(On Diff #242698)

nit:
The #1 can be removed because it no longer refers to anything.

70 ↗(On Diff #242698)

nit:
Did you mean to use #0 here instead?

102 ↗(On Diff #242698)

nit:
The #1 can be removed here too.

nemanjai marked 5 inline comments as done.Feb 7 2020, 5:27 AM

Thanks for the comments, I'll address them and upload an update shortly.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
170

Hmm... yeah, I think that's a good question. Presumably with soft float, we shouldn't have a problem since everything is a libcall with GPR parameters, but I'll double check.

llvm/lib/Target/PowerPC/PPCInstrVSX.td
114

I believe that no other targets support extending loads and truncating stores for this type. I can move it to the target independent file for consistency though.

llvm/test/CodeGen/PowerPC/handle-f16-storage-type.ll
35 ↗(On Diff #242698)

OK.

36 ↗(On Diff #242698)

OK.

70 ↗(On Diff #242698)

Yes. Thanks.

nemanjai updated this revision to Diff 243150.Feb 7 2020, 5:57 AM

Fixed up the nits in the test cases and added a test for soft float. Also, moved the pattern fragments to the target independent td file for consistency.

jsji accepted this revision as: jsji.Feb 7 2020, 7:57 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Feb 7 2020, 7:57 AM

Ping? Is this ready to land?

Ping? Is this ready to land?

Oh, sorry I kind of forgot about this. Just running the test and I'll commit it if everything still checks out. Thanks for reminding me.

This revision was automatically updated to reflect the committed changes.