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.
Details
- Reviewers
nemanjai saghir hfinkel amyk - Group Reviewers
Restricted Project - Commits
- rG331dcc43eac2: [PowerPC] Implemented Vector Load with Zero and Signed Extend Builtins
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
---|---|---|
16562 | Nit: space after this comment. | |
16565 | 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). | |
16583 | Nit: space after this comment. | |
17045 | Remove unrelated whitespace change. | |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
13646 | Move this comment above the combine function since this comment was intended to describe the function. | |
13653 | Can omit the { } since we just have a single return underneath. | |
13660 | Can omit the { } since we just have a single return underneath. | |
llvm/lib/Target/PowerPC/PPCInstrPrefix.td | ||
1323 | 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. |
clang/test/CodeGen/builtins-ppc-p10vector.c | ||
---|---|---|
12 | 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 ? |
Please move encoding tests to ppc64-encoding-ISA31.[txt|s].
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
13671 | nit: check ValidLDType first: if (! ValidLDType || (LD->getValueType(0) != MVT::i128) || (LD->getExtensionType() != ISD::ZEXTLOAD) ||) | |
llvm/lib/Target/PowerPC/PPCISelLowering.h | ||
544 | 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 | ||
531 | nit: indentation |
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 |
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 |
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. |
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? |
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.
I think the patterns for the load instructions may have accidentally been deleted. Please add them back to the patch. Thank you.
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 | ||
---|---|---|
16564 | I'm a little confused about this. | |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
13671 | It looks like you are doing this for zero extend only. | |
llvm/test/CodeGen/PowerPC/builtins-ppc-p10vsx.ll | ||
60 | nit: |
Addressed formatting comments
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
13664 | I'm not too sure what you mean, would you be able to elaborate?
Thanks! |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
13664 | Extra the in your sentence: |
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. |
Clang formatted relevant lines, combined LE and BE tests as they produced the same ASM.
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.
Nit: space after this comment.