Page MenuHomePhabricator

[PowerPC][Builtins] Added a number of builtins for compatibility with XL.

Authored by stefanp on Jun 16 2021, 7:53 AM.



Added a number of different builtins that exist in the XL compiler. Most of
these builtins already exist in clang under a different name.

Diff Detail

Event Timeline

stefanp created this revision.Jun 16 2021, 7:53 AM
stefanp requested review of this revision.Jun 16 2021, 7:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2021, 7:53 AM
stefanp added reviewers: rsmith, Restricted Project.Jun 16 2021, 7:55 AM
qiucf added a subscriber: qiucf.Jun 16 2021, 11:55 PM
qiucf added inline comments.

For these tests against builtin mapping to an intrinsic, could we only keep the call line to make it simpler? (as D103986 does)

lei added inline comments.Wed, Jul 14, 6:40 AM

I think you need sema checking for parm 1.

Must be a constant integer with a value greater than zero and of a power of two.

sema checking?

stefanp updated this revision to Diff 358915.Thu, Jul 15, 4:04 AM

Sorry for letting this patch sit for so long.

I have updated the tests to simplify them so that we only include the
intrinsics (As per a comment). This reduces quite a bit of clutter.

Added Sema Checking for the builtins that were created.

stefanp updated this revision to Diff 359054.Thu, Jul 15, 11:00 AM

Realized after I updated the patch that I got the darn tests wrong.
I've updated the patch and fixed that.

Can the test cases that just check for specific IR being produced be merged together? Seems unnecessary to have all of these separate test cases.
Perhaps something along the lines of:

  • Test case for diagnostics of invalid use
  • Test case for produced IR
  • Test case for specific metadata being produced
9732 ↗(On Diff #359054)

I think this comes from another patch that is up for review. You should base this patch on top of that patch and mark the review as a dependency. It makes the review easier if the review only contains code that is meant to go in this commit.


Are these two just Ops[0], Ops[1]?


Same comment as above.


Should this not test for the meta data that __builtin_expect produces?

stefanp updated this revision to Diff 359459.Fri, Jul 16, 2:46 PM

Addressed a number of comments.

Replaced with Ops[0], Ops[1] where possible.
Rebased on top of the patch that defines SemaValueIsRunOfOnes. That patch is now
comitted anyway.

Merged a number of the test files. I was not able to merge all of the test files
as some of the builtins required Power 8 and up or Power 9 and up. Did not merge
existing files. Also did not merge the complex test file as it requires a
different check for LE, BE, and AIX.

stefanp marked an inline comment as not done.Fri, Jul 16, 2:48 PM
stefanp added inline comments.
9732 ↗(On Diff #359054)

Yes it does. That patch has actually already gone in so I've just rebased on top of it and solved this issue that way.


Yes they are. I will replace where possible.

nemanjai accepted this revision.Mon, Jul 19, 9:18 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Mon, Jul 19, 9:18 AM
This revision was landed with ongoing or failed builds.Tue, Jul 20, 6:58 AM
This revision was automatically updated to reflect the committed changes.