This patch is in a series of patches to provide builtins for compatibility
with the XL compiler. This patch adds the builtins and instrisics for compare
and multiply related operations.
Details
- Reviewers
nemanjai stefanp - Group Reviewers
Restricted Project - Commits
- rG18c19414eb70: [PowerPC] Add PowerPC compare and multiply related builtins and instrinsics for…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
- Renamed the XLCompat builtin as __builtin_ppc_* and add them to definedXLCompatMacros and update the test cases.
- Report error in SemaChecking when 64 bit only builtins run on a 32 bit target and update the test cases.
- Move the XLCompat Intrinsics definition under
let TargetPrefix = "ppc" in { ... }
- Add AIX 32&64 bit run line checks (front and back end test cases)
- Create builtin-ppc-xlcompat-error.c for arguments related error check, add error test case for __builtin_ppc_cmprb
- Remove 32 bit linux run line checks
- Update bulitin test cases to remove redundant definitions.
clang/lib/Basic/Targets/PPC.h | ||
---|---|---|
354 ↗ | (On Diff #352560) | Can you pleases rebase your patch again? The removal of this function was part of D104125. |
clang/test/CodeGen/builtins-ppc-xlcompat-compare-64bit-only.c | ||
2 ↗ | (On Diff #352560) | Are these pwr9 specific as well? If not please targe pwr7 for big endian and pwr8 for little endian |
clang/test/CodeGen/builtins-ppc-xlcompat-multiply-64bit-only.c | ||
3 | same comment as above. |
clang/test/CodeGen/builtins-ppc-xlcompat-compare-64bit-only.c | ||
---|---|---|
6 ↗ | (On Diff #352560) | why not just use default for 64BIT behaviour? No need for --check-prefix=CHECK-64 |
clang/test/CodeGen/builtins-ppc-xlcompat-compare-64bit-only.c | ||
---|---|---|
8 ↗ | (On Diff #352560) | -check-prefix=CHECK-32 => --check-prefix=CHECK-32 |
- Added Sema check for the pwr9 only and 64 bit only builtins and updated the test cases.
- Rebased the patch with ToT.
- Cleaned up the test cases and address review comments
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-compare-64bit-only.ll | ||
---|---|---|
40 | Are the attributes needed? | |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-compare.ll | ||
42 | Same question here (and the remaining tests). | |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-multiply-64bit-only.ll | ||
8 | Does it make sense to add pre-P9 for mulhd/mulhdu since they existed prior to P9? | |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-multiply.ll | ||
10 | Does it make sense to add pre-P9 for these instructions that existed prior to P9? |
Address review comments on the test case. Target cpu sema checking covered in front end test cases. will keep current coverage in backend test.
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
3372 | This is just a question. | |
clang/test/CodeGen/builtins-ppc-xlcompat-multiply-64bit-only.c | ||
16 | Does entry need to be removed from these tests (in case of failing in non-assert builds)? | |
llvm/include/llvm/IR/IntrinsicsPowerPC.td | ||
1569 | nit: unrelated change? |
Address review comments on test case and remove change not needed.
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
3372 | yeah, we planned using this feature to do the sema check for pwr9 only (or later cpus) builtins. |
Need to merge with https://reviews.llvm.org/D105501 changes once approved for pwr9 (or later processor) only sema checking.
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-multiply.ll | ||
---|---|---|
10 | Will merge in Power ISA features Sema checking patch https://reviews.llvm.org/D105501 for pwr9+ only sema checking once approved. |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
9754 ↗ | (On Diff #357625) | No longer required after https://reviews.llvm.org/D105501 |
clang/lib/Sema/SemaChecking.cpp | ||
3372 | Now that https://reviews.llvm.org/D105501 is approved, please change this to | |
llvm/include/llvm/IR/IntrinsicsPowerPC.td | ||
1568 | Shouldn't operand 0 be marked as an immediate here? | |
llvm/lib/Target/PowerPC/PPCInstrInfo.td | ||
5264 | Shouldn't $a be a u1imm? |
LGTM aside from a formatting nit.
llvm/include/llvm/IR/IntrinsicsPowerPC.td | ||
---|---|---|
1574 | Nit: This line looks too long. Some others below as well. |
This is just a question.
Is power9-vector the correct feature check in these cases? Does it matter if these are not vector instructions?