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

[Conanap]

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.

[Conanap]

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]

Fixed some formatting stuff.

[Conanap]

Addressed Lei's comments

[lei]

llvm/lib/Target/PowerPC/PPCISelLowering.h

501

missing ,?

llvm/lib/Target/PowerPC/PPCInstrPrefix.td

526

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

// PowerPC specific type constraints.

530

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]

Fixed a missing comma; ensured it builds.

[Conanap]

fixed a missing comma and ensure that it compiles

[Conanap]

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

[Conanap]

Addressed Lei's comments

[lei]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

13670

nit: don't need () aroud !ValidLDType

13676

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...

13677

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]

Now depends on D83364.

[Conanap]

Also removed unnecessary brackets and comments.

[amyk]

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

9

unintended change?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

13675

You did not assign anything here?

[Conanap]

Removed unecessary comments and unintended changes.

[Conanap]

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]

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

[Conanap]

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

[stefanp]

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

16564

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

13671

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

60

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]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

13664

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

llvm/lib/Target/PowerPC/PPCInstrPrefix.td

29

nit: one space is enough.

[Conanap]

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

[lei]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

13674

nit: indentation.

13677

nit: indentation

[Conanap]

Addressed formatting comments

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

13664

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]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

13664

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]

Included sext test case.

[amyk]

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

13674

Over 80 characters?

llvm/test/CodeGen/PowerPC/builtins-ppc-p10vsx.ll

6

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]

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

[NeHuang]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

13678

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

llvm/test/CodeGen/PowerPC/builtins-ppc-p10vsx.ll

4

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

[Conanap]

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

[amyk]

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

920

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

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

13746

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

[amyk]

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]

LGTM aside from a couple of minor nits.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

13651

This line is redundant. Please remove it.

13670

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