This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Support constrained vector fp/int conversion
ClosedPublic

Authored by qiucf on Jul 12 2020, 9:56 PM.

Details

Summary

This patch makes these operations legal, and add necessary codegen patterns.

There's still some issue similar to D77033 for conversion from/to v1i128, but normal type tests synced from X86/SystemZ's vector-constrained-fp-intrinsics.ll are all okay.

Diff Detail

Event Timeline

qiucf created this revision.Jul 12 2020, 9:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2020, 9:56 PM
uweigand added inline comments.Jul 14 2020, 4:00 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
8638

It would seem that this doesn't create a correct node when called with a STRICT version of Opc -- for one, in this case it will need to handle chains correctly (in and out).

It is strange that this was not detected by any tests -- is the coverage good enough?

The rest of the algorithm seems OK for the strict case, since it only introduces integer operations.

qiucf updated this revision to Diff 278696.Jul 17 2020, 2:57 AM

Fix incorrect operand in LowerINT_TO_FPVector.

qiucf marked 2 inline comments as done.Jul 17 2020, 3:08 AM
qiucf added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
8638

Yes, it was missed here. Thanks for comments!

This method is for custom lowering v4i8/v4i16. They are not set to custom in this patch so such cases will be expanded automatically. After setting them, it hits an assert since PPC doesn't override TargetLowering::LowerOperationWrapper. This can be done in later patches.

if (SDValue Res = LowerOperation(SDValue(N, 0), DAG))
  Results.push_back(Res);

BTW, the method looks strange to me, maybe we can fill rest of operands in base class implementation.

uweigand added inline comments.Jul 17 2020, 3:47 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
8638

Well, if the changes to this routine are never exercised without some additional changes, maybe they should be removed from this patch and added to the patch that'll introduce the first use?

On the other hand, I see above that at least for v4i1 the method is set to Custom, so is this routine used after all? (Not sure when v4i1 is actually used here, a bool->float conversion looks rare ... Are there test cases for v4i1?)

qiucf updated this revision to Diff 279434.Jul 20 2020, 11:53 PM
qiucf marked 3 inline comments as done.

Add handling for v4i16 and tests

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

Oh, v4i1 would be promoted to v4i32. So setting it to custom (both strict and non-strict) is actually meaningless.. I'll add the handling for v4i16 and others along with tests.

uweigand accepted this revision.Jul 21 2020, 12:46 AM

This LGTM now.

This revision is now accepted and ready to land.Jul 21 2020, 12:46 AM
This revision was landed with ongoing or failed builds.Aug 23 2020, 7:19 PM
This revision was automatically updated to reflect the committed changes.
llvm/lib/Target/PowerPC/PPCInstrVSX.td