Page MenuHomePhabricator

[PowerPC] Use xxleqv to set all one vector IMM(-1).
ClosedPublic

Authored by jsji on Jul 31 2019, 11:58 AM.

Details

Summary

xxspltib/vspltisb are 3 cycle PM instructions,
xxleqv is 2 cycle ALU instruction.

We should use xxleqv to set all one vectors.

Diff Detail

Repository
rL LLVM

Event Timeline

jsji created this revision.Jul 31 2019, 11:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2019, 11:58 AM
wuzish added inline comments.Jul 31 2019, 10:21 PM
llvm/lib/Target/PowerPC/PPCInstrVSX.td
1298 ↗(On Diff #212635)

Has immAllOnesV contained bitcast semantic? It's equal to ISD::isBuildVectorAllOnes.

4079 ↗(On Diff #212635)

If we don't enum all the type, just use v4i32 version. Are other types bitcast/canonical into v4i32 automatically?

jsji marked 2 inline comments as done.Aug 1 2019, 6:29 AM
jsji added inline comments.
llvm/lib/Target/PowerPC/PPCInstrVSX.td
1298 ↗(On Diff #212635)

No, it doesn't.

4079 ↗(On Diff #212635)

We will canonize isBuildVectorAllOnes into v16i8, we don't need to enum all types.

steven.zhang requested changes to this revision.Aug 14 2019, 12:31 AM

Only v4i32 is handled. We need to handle all the valid vector type.

llvm/lib/Target/PowerPC/PPCInstrFormats.td
1212 ↗(On Diff #212635)

Can you commit another NFC patch to do the rename and the XX3Form_SetZero seems to be completely the same as this one. I have no idea why we have two forms. We need to remove it.

llvm/lib/Target/PowerPC/PPCInstrVSX.td
1298 ↗(On Diff #212635)

You are matching the pattern for v4i32, but miss to catch the other type of pattern. i.e. v8i16 etc. We won't canonical all the vector type to v4i32 during the type legalize. Even we do that, it is better to add all the match rules in td file as we cannot depend on the legalization implementation.

Your patch cannot handle the other vector type except the v4i32.

define dso_local <8 x i16> @_Z3foov() local_unnamed_addr #0 {
entry:
  ret <8 x i16> <i16 -1, i16 -1, i16 -1, i16 -1,i16 -1, i16 -1, i16 -1, i16 -1>
}

We need add some pattern like:

def : Pat<(v8i16 (bitconvert (v16i8 immAllOnesV))),
            (v8i16 (COPY_TO_REGCLASS(XXLEQVOnes), VSRC))>;
...
2420 ↗(On Diff #212635)

extra space.

llvm/test/CodeGen/PowerPC/build-vector-tests.ll
761 ↗(On Diff #212635)

Add some tests to test v1i128, v2i64, v8i16 v16i8

This revision now requires changes to proceed.Aug 14 2019, 12:31 AM
jsji marked 5 inline comments as done.Aug 14 2019, 7:19 AM
llvm/lib/Target/PowerPC/PPCInstrFormats.td
1212 ↗(On Diff #212635)
llvm/lib/Target/PowerPC/PPCInstrVSX.td
1298 ↗(On Diff #212635)

I should say, isBuildVectorAllOnes can see through bitcast, the bitconvert here is mostly the trick to get complexity up, so that we can prefer this over other patterns.

1298 ↗(On Diff #212635)

Agree. Will do that.

4079 ↗(On Diff #212635)

Ah, my bad, yes, you are right, I mis-read the condition of lowering, we did not really canonize.

llvm/test/CodeGen/PowerPC/build-vector-tests.ll
761 ↗(On Diff #212635)

I will add those in another file, this file is far too big.

jsji marked an inline comment as done.Aug 14 2019, 7:54 AM
jsji added inline comments.
llvm/test/CodeGen/PowerPC/build-vector-tests.ll
761 ↗(On Diff #212635)

Added a new test file for different vector types in https://reviews.llvm.org/rL368859

jsji updated this revision to Diff 215233.Aug 14 2019, 1:45 PM

Added patterns for other type and rebased to pick up new tests.

steven.zhang accepted this revision.Aug 15 2019, 1:00 AM

LGTM except some minor comments.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
390 ↗(On Diff #215233)

line too long. immAnyExt8NonAllOnes or immNonAllOnesAnyExt8?

llvm/lib/Target/PowerPC/PPCInstrVSX.td
1328 ↗(On Diff #215233)

Can we put these pattern in line 4112 to match the build vector in the same place ?

This revision is now accepted and ready to land.Aug 15 2019, 1:00 AM
wuzish added inline comments.Aug 15 2019, 1:30 AM
llvm/lib/Target/PowerPC/PPCInstrInfo.td
390 ↗(On Diff #215233)

I think no need to modify this to exclude all ones. So long as its position which is blow all ones Pat, it would not be selected before all ones. Can have a try.

jsji marked an inline comment as done.Aug 15 2019, 6:48 AM
jsji added inline comments.
llvm/lib/Target/PowerPC/PPCInstrInfo.td
390 ↗(On Diff #215233)

No, ISEL select the pattern according to complexity , not the position of where the pattern defined.
And yes, I did try before modifying this pattern, if we don't exclude all ones here, this pattern will be selected first, as it is complexity is far larger than others.
eg: 477 vs. 407.

jsji updated this revision to Diff 215396.Aug 15 2019, 7:27 AM
jsji marked an inline comment as done.

Fix long line and move the patterns.

This revision was automatically updated to reflect the committed changes.