This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Extend custom lower of vector truncate to handle wider input
ClosedPublic

Authored by RolandF on Sep 25 2019, 9:43 AM.

Details

Summary

Current custom lowering of truncate vector handles a source of up to 128 bits, but that only uses one of the two shuffle vector operands. Extend it to use both operands to handle 256 bit sources.

Diff Detail

Event Timeline

RolandF created this revision.Sep 25 2019, 9:43 AM

Is it possible to check the permutation mask in the test case? (I suppose that these are auto-generated tests, but the test coverage it's all that good if we can't check the mask).

RolandF updated this revision to Diff 222174.Sep 27 2019, 8:04 AM

Moved the tests to a separate file and added hand-written checks which include the permute control data.

qiucf added a subscriber: qiucf.Oct 15 2019, 1:45 AM
qiucf added inline comments.
llvm/test/CodeGen/PowerPC/vec-trunc2.ll
19

Use CHECK-COUNT?

RolandF updated this revision to Diff 225056.Oct 15 2019, 9:36 AM

Update test to use CHECK-COUNT.

RolandF marked an inline comment as done.Oct 15 2019, 9:37 AM
nemanjai added inline comments.Jul 15 2020, 3:17 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7166

I believe DAG.getTargetLoweringInfo() is just *this so we should be able to just call getVectorIdxTy() unqualified.

7169

Isn't SplitVT just SrcVT.getHalfNumVectorElementsVT(*DAG.getContext())?

Seems that all of the definitions here can just be something like:

EVT VecIdxTy = getVectorIdxTy(DAG.getDataLayout());
EVT SplitVT = N1.getValueType().getHalfNumVectorElementsVT(*DAG.getContext());
unsigned SplitNumElts = SplitVT.getVectorNumElements();
10208

We check that the number of elements in the wide input is a power of 2 as well as that the target element type width is a power of 2.
What happens with something weird like trunc <8 x i24> to <8 x i16> (also, please add such a test case)?

llvm/test/CodeGen/PowerPC/vec-trunc2.ll
21

Please add a check that shows how we produced v3. Presumably this is a shift from v2?

RolandF updated this revision to Diff 279948.Jul 22 2020, 2:55 PM

Improved testing and tightened checks.

RolandF marked 3 inline comments as done.Jul 22 2020, 2:58 PM

Resolve some comments.

RolandF marked an inline comment as done.Jul 22 2020, 3:04 PM

Resolve comment.

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

Moved checks to lowering function. There was an existing check for source bit width. so 8x24 was skipped.. Added some additional checking. Added an 8x24 test to the test case.

nemanjai accepted this revision.Aug 20 2020, 3:41 AM

Other than a couple of minor nits, LGTM. I am so sorry about the delay in reviewing this.

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

I realize that we don't need to bitcast Op1 here because it already gets bitcasted below. But I think it is one of those things that a reader looking at this code thinks "Huh? How come only one is bitcasted?" (until they get to the bottom of the function).

So my minor nit here would be to just do both bicasts at the same point below unconditionally. Bitcasting to the same type should not produce a new node.

llvm/test/CodeGen/PowerPC/vec-trunc2.ll
66

It's bizarre that the script put the checks inside the signature of the function.

This revision is now accepted and ready to land.Aug 20 2020, 3:41 AM