This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Implement Load VSX Vector and Sign Extend and Zero Extend
ClosedPublic

Authored by Conanap on Jun 24 2020, 2:24 PM.

Details

Summary

Implemented Builtins for Load VSX Vector and sign extend and zero extend along with appropriate test cases. The new instructions are used for ZEXT but SEXT will continue to use the previously generated ASM.

Diff Detail

Event Timeline

Conanap created this revision.Jun 24 2020, 2:24 PM
amyk requested changes to this revision.Jun 24 2020, 2:48 PM
amyk added a subscriber: amyk.

I have a few comments from the last time we looked at this together.
Just also FYI that the backend tests will be apart of the p10-vsx-builtins.ll file introduced in https://reviews.llvm.org/D82431.

clang/lib/Headers/altivec.h
16514

Nit: space after this comment.

16517

I realized that I think we are supposed to use unaligned_vec_si128 and unaligned_vec_ui128 instead of vector signed __int128 and vector unsigned __int128 when we return (for here, and other places).

16535

Nit: space after this comment.

16917

Remove unrelated whitespace change.

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

Move this comment above the combine function since this comment was intended to describe the function.

14143

Can omit the { } since we just have a single return underneath.

14150

Can omit the { } since we just have a single return underneath.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
976

Can remove this comment.

llvm/test/CodeGen/PowerPC/p10-vsx-builtins.ll
6 ↗(On Diff #273159)

Please update this comment to be more descriptive. Maybe something like,

These test cases tests that zero extending loads utilize the Load VSX Vector Rightmost (lxvr[b|h|w|d]) instructions in Power10.
8 ↗(On Diff #273159)

Please remove the comments/checks above the functions.

This revision now requires changes to proceed.Jun 24 2020, 2:48 PM
Conanap updated this revision to Diff 273393.Jun 25 2020, 8:54 AM

Addressed Amy's comments regarding documentation of the changes.

Conanap updated this revision to Diff 273421.Jun 25 2020, 9:56 AM
Conanap marked 9 inline comments as done.

Fixed return signature for the open coded functions in altivec.h

anil9 added a subscriber: anil9.Jun 25 2020, 4:21 PM
anil9 added inline comments.
clang/test/CodeGen/builtins-ppc-p10vector.c
17

nit: It seems that most pull requests follow an ordering like first signed declaration and then unsigned, declaration, this one follows too , except the above two lines. And should the above declarations of chars, be along with these lines ?

llvm/test/CodeGen/PowerPC/p10-vsx-builtins.ll
56 ↗(On Diff #273421)

I am not too familiar with the builtins but I never saw a check outside of the two braces in the test cases before, is it not posible to include it inside the test cases ?

Conanap updated this revision to Diff 274097.Jun 29 2020, 6:59 AM
Conanap marked an inline comment as done.

Reordered some declarations in test cases and removed unecessary extra CHECKs.

lei added a subscriber: lei.Jun 30 2020, 4:06 PM

Please move encoding tests to ppc64-encoding-ISA31.[txt|s].

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

nit: check ValidLDType first:

if (! ValidLDType || (LD->getValueType(0) != MVT::i128) ||
    (LD->getExtensionType() != ISD::ZEXTLOAD) ||)
llvm/lib/Target/PowerPC/PPCISelLowering.h
547

nit: This shouldn't matter to code gen, but it would be nice to have this defined closer to other similar instructions. Maybe after LXVD2X?

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
453

nit: indentation

Conanap marked 2 inline comments as done.Jul 2 2020, 7:36 AM

Addressed Anil's comments with regards to the test cases

Conanap added inline comments.Jul 2 2020, 7:36 AM
llvm/test/CodeGen/PowerPC/p10-vsx-builtins.ll
56 ↗(On Diff #273421)

these were extra checks that I should've removed; thanks for catching it

Conanap updated this revision to Diff 275124.Jul 2 2020, 7:52 AM
Conanap marked 4 inline comments as done.

Fixed some formatting stuff.

Addressed Lei's comments

lei added inline comments.Jul 2 2020, 9:33 AM
llvm/lib/Target/PowerPC/PPCISelLowering.h
500

missing ,?

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
448

We should put all such def at the top of this file under:

// PowerPC specific type constraints.
452

Put these under the type constraints section and doc with:

// PowerPC specific DAG Nodes.
llvm/test/CodeGen/PowerPC/p10-vsx-builtins.ll
3 ↗(On Diff #275124)

remve extra line. Makes the comment harder to read.

10 ↗(On Diff #275124)

You don't need the local_unnamed_addr #0

Conanap updated this revision to Diff 275159.Jul 2 2020, 10:42 AM

Fixed a missing comma; ensured it builds.

Conanap marked an inline comment as done.Jul 2 2020, 10:43 AM

fixed a missing comma and ensure that it compiles

Conanap updated this revision to Diff 275423.Jul 3 2020, 10:05 AM
Conanap marked 3 inline comments as done.

Moved some code to the top of the file as per Lei's request

Conanap marked 2 inline comments as done.Jul 3 2020, 10:05 AM

Addressed Lei's comments

lei added inline comments.Jul 7 2020, 6:45 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
14160

nit: don't need () aroud !ValidLDType

14166

I don't think we need to explicitly say this sine everything we do here is for pattern matching to appropriate instructions in the backend...

14167

Can just merge this into the next line and remove this tmp value.

llvm/test/CodeGen/PowerPC/p10-vsx-builtins.ll
6 ↗(On Diff #275423)

Please add tests for BE.

Conanap updated this revision to Diff 276785.Jul 9 2020, 11:25 AM
Conanap marked 3 inline comments as done.

Now depends on D83364.

Also removed unnecessary brackets and comments.

amyk added a comment.Jul 10 2020, 8:39 AM

Please update this patch to remove the instruction defs and MC tests. Also, you can update the patch to put your backend llc tests in the file I've introduced in: https://reviews.llvm.org/D82467

clang/test/CodeGen/builtins-ppc-p10vector.c
14

unintended change?

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

You did not assign anything here?

Conanap updated this revision to Diff 277415.Jul 13 2020, 6:57 AM
Conanap marked 2 inline comments as done.

Removed unecessary comments and unintended changes.

Conanap updated this revision to Diff 277418.Jul 13 2020, 7:01 AM

Removed duplicate test code found in the instructions and MC Test implementation of VSX Vector store and load with sign extend or zero extend; re-added a test file that was omitted in the last diff update.

amyk added a comment.Jul 14 2020, 11:10 AM

I think the patterns for the load instructions may have accidentally been deleted. Please add them back to the patch. Thank you.

Conanap updated this revision to Diff 277952.Jul 14 2020, 1:06 PM

Restored accidentally deleted pattern, removed duplicate tests, moved new tests to another pre-existing file instead.

The title of the patch mentions both zero extend and sign extend.
However, it seems that we only have instructions for the zero extend case. Is that right?
I see both types of tests in:
test/CodeGen/builtins-ppc-p10vector.c
But I only see codegen tests for the zreo extend version.
test/CodeGen/PowerPC/builtins-ppc-p10vsx.ll

I bring it up because I want to make sure that this is intentional.

clang/lib/Headers/altivec.h
16516

I'm a little confused about this.
Your function signature says we return vector unsigned __int128 but the return statement casts to unaligned_vec_si128. Is that how this is supposed to look?

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

It looks like you are doing this for zero extend only.
What about ISD::EXTLOAD? In that case the upper bits are undefined anyway so we can just assume a zero extend right?

llvm/test/CodeGen/PowerPC/builtins-ppc-p10vsx.ll
41 ↗(On Diff #277952)

nit:
Was this line auto-added?
Usually this comment is added at the top of the file by the script. I don't think it is required here.

NeHuang added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
14154

nit: if we are loading either a byte....

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
29

nit: one space is enough.

Conanap updated this revision to Diff 279362.Jul 20 2020, 3:54 PM
Conanap marked 5 inline comments as done.

Return signature fix, added recognition for ISD:EXTLOAD, some code clean up.

lei added inline comments.Jul 23 2020, 5:23 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
14164

nit: indentation.

14167

nit: indentation

Conanap updated this revision to Diff 280597.Jul 24 2020, 3:01 PM
Conanap marked 2 inline comments as done.
Conanap retitled this revision from [PowerPC][Power10] Implement Load VSX Vector and Sign Extend and Zero Extend to [PowerPC] Implement Load VSX Vector and Sign Extend and Zero Extend.
Conanap edited the summary of this revision. (Show Details)

Addressed formatting comments

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

I'm not too sure what you mean, would you be able to elaborate?
The comment is:

This transformation is only valid if the we are loading either a byte,
halfword, word, or doubleword.

Thanks!

lei added inline comments.Jul 27 2020, 6:18 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
14154

Extra the in your sentence:
This transformation is only valid if the we are loading -> This transformation is only valid if we are loading

Conanap updated this revision to Diff 281035.Jul 27 2020, 1:08 PM

Included sext test case.

amyk added a comment.Jul 27 2020, 9:31 PM

Could we also elaborate in the description on how we are utilizing the new load instructions for zero extend case but not the sign extend case?

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

Over 80 characters?

llvm/test/CodeGen/PowerPC/builtins-ppc-p10vsx.ll
5 ↗(On Diff #281035)

This line actually is for -O0 and not a BE RUN line. I believe the most up to date version of the test includes the LE, BE and O0 line.

Conanap updated this revision to Diff 281974.Jul 30 2020, 10:29 AM
Conanap marked 2 inline comments as done.
Conanap removed a reviewer: power-llvm-team.

Some updates on formatting and updated to match an updated test file.

Conanap edited the summary of this revision. (Show Details)Jul 30 2020, 11:00 AM
NeHuang added inline comments.Jul 30 2020, 11:24 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
14168

please clang-format the changed files before updating the patch.

llvm/test/CodeGen/PowerPC/builtins-ppc-p10vsx.ll
4 ↗(On Diff #281974)

Why do we need different check prefixes for LE and BE? They produce the same asm. It seems we only need CHECK-O0.

Conanap updated this revision to Diff 283061.Aug 4 2020, 4:43 PM
Conanap marked 2 inline comments as done.

Clang formatted relevant lines, combined LE and BE tests as they produced the same ASM.

amyk accepted this revision.Aug 6 2020, 7:58 AM
amyk added inline comments.
clang/test/CodeGen/builtins-ppc-p10vector.c
590

It would be good to be a little more specific with the CHECK lines.

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

This line here and below looks like unintended changes from clang-format I am guessing? I am OK with them being removed during commit.

This revision is now accepted and ready to land.Aug 6 2020, 7:58 AM
amyk added a comment.Aug 7 2020, 10:50 AM

I realized I didn't put a comment on this earlier but this overall LGTM, but I think it would be good to see if @nemanjai has any additional comments on this patch.

nemanjai accepted this revision.Aug 21 2020, 12:08 PM

LGTM aside from a couple of minor nits.

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

This line is redundant. Please remove it.

14160

The second condition was already checked on line 13604. You can omit it here.

This revision was landed with ongoing or failed builds.Aug 28 2020, 9:29 AM
This revision was automatically updated to reflect the committed changes.
Conanap marked an inline comment as done.