This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Add PowerPC compare and multiply related builtins and instrinsics for XL compatibility
ClosedPublic

Authored by NeHuang on May 20 2021, 2:21 PM.

Details

Summary

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.

Diff Detail

Event Timeline

NeHuang created this revision.May 20 2021, 2:21 PM
NeHuang requested review of this revision.May 20 2021, 2:21 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 20 2021, 2:21 PM
NeHuang edited the summary of this revision. (Show Details)May 20 2021, 2:23 PM
NeHuang added reviewers: nemanjai, stefanp, Restricted Project.
NeHuang updated this revision to Diff 350991.Jun 9 2021, 5:40 PM
  • 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 {
...
}
NeHuang updated this revision to Diff 351989.Jun 14 2021, 1:49 PM

Rebased the patch with changes in https://reviews.llvm.org/D104125

NeHuang updated this revision to Diff 352560.Jun 17 2021, 6:49 AM
  • 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.
lei added a subscriber: lei.Jun 17 2021, 7:08 AM
lei added inline comments.
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.

lei added inline comments.Jun 17 2021, 11:47 AM
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

lei added inline comments.Jun 17 2021, 11:49 AM
clang/test/CodeGen/builtins-ppc-xlcompat-compare-64bit-only.c
8 ↗(On Diff #352560)

-check-prefix=CHECK-32 => --check-prefix=CHECK-32

NeHuang updated this revision to Diff 354081.EditedJun 23 2021, 2:34 PM
  • 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
NeHuang marked an inline comment as done.Jun 23 2021, 2:36 PM
amyk added a subscriber: amyk.Jul 5 2021, 6:23 AM
amyk added inline 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?

NeHuang updated this revision to Diff 356701.Jul 6 2021, 6:34 AM
NeHuang marked 4 inline comments as done.

Address review comments on the test case. Target cpu sema checking covered in front end test cases. will keep current coverage in backend test.

gentle ping.

amyk added inline comments.Jul 8 2021, 4:26 PM
clang/lib/Sema/SemaChecking.cpp
3372

This is just a question.
Is power9-vector the correct feature check in these cases? Does it matter if these are not vector instructions?

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)?
Perhaps it may not fail but might be good to double check.

llvm/include/llvm/IR/IntrinsicsPowerPC.td
1569

nit: unrelated change?

NeHuang updated this revision to Diff 357625.Jul 9 2021, 1:51 PM
NeHuang marked 3 inline comments as done.

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.

NeHuang added inline comments.
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.

nemanjai added inline comments.Jul 13 2021, 2:57 AM
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
isa-v30-instructions please.

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?

nemanjai requested changes to this revision.Jul 13 2021, 7:15 AM

Requesting changes to keep the queue accurate.

This revision now requires changes to proceed.Jul 13 2021, 7:15 AM
NeHuang updated this revision to Diff 358397.Jul 13 2021, 12:53 PM
NeHuang marked 4 inline comments as done.

Addressed review comments from Nemanja.

nemanjai accepted this revision.Jul 13 2021, 2:19 PM

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 revision is now accepted and ready to land.Jul 13 2021, 2:19 PM
This revision was landed with ongoing or failed builds.Jul 13 2021, 2:55 PM
This revision was automatically updated to reflect the committed changes.