Page MenuHomePhabricator

[PowerPC] Floating Point Builtins for XL Compat.
ClosedPublic

Authored by quinnp on Jun 9 2021, 12:44 PM.

Details

Summary

This patch is in a series of patches to provide
builtins for compatibility with the XL compiler.
This patch adds builtins related to floating point
operations

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 9 2021, 12:44 PM
quinnp edited the summary of this revision. (Show Details)Jun 9 2021, 12:54 PM
quinnp added reviewers: Restricted Project, nemanjai, stefanp.
quinnp edited the summary of this revision. (Show Details)Jun 10 2021, 8:31 AM
quinnp edited the summary of this revision. (Show Details)
quinnp updated this revision to Diff 352240.Jun 15 2021, 2:15 PM

Changed the target architecture of tests to follow the convention: BE-pwr7 LE-pwr8.

qiucf added a subscriber: qiucf.Jun 16 2021, 11:51 PM
qiucf added inline comments.
clang/test/CodeGen/builtins-ppc-xlcompat-sync.c
2 ↗(On Diff #352240)

Why change pwr7 to pwr8?

llvm/lib/Target/PowerPC/PPCInstrInfo.td
3341

Seems we didn't exploit XXSEL in this case. But for sqrt/rsqrt, PPC has VSX and non-VSX versions for them:

frsqrte - xsrsqrtedp
frsqrtes - xsrsqrtesp
fsqrt - xssqrtdp
fsqrts - xssqrtsp

So needs to add frsqrte here and xsrsqrtesp/xssqrtsp in VSX part?

llvm/test/CodeGen/builtins-ppc-xlcompat-fp.ll
10

You can use update_llc_test script to automatically update this file.

And add non-vsx test?

quinnp updated this revision to Diff 353475.EditedJun 21 2021, 1:52 PM

Addressing some comments and updating tests

quinnp updated this revision to Diff 353716.Jun 22 2021, 11:04 AM
quinnp marked 2 inline comments as done.

Fixing the 32bit AIX run line in the testcases.

quinnp updated this revision to Diff 354498.Jun 25 2021, 7:35 AM

Added non-vsx implementation of builtins and non-vsx backend tests

Overall looks good. Some nits as below.

clang/test/CodeGen/builtins-ppc-xlcompat-fp.c
10
  • You can define three extern variables for all the bulitins.
extern double a;
extern float b;
extern float c;
  • You can auto update the test case with utils/update_cc_test_checks.py
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4986

you can delete blank line and better add comments for the operation below.

4988

Please use Ops as the variable name.

NeHuang added inline comments.Wed, Jun 30, 1:29 PM
llvm/test/CodeGen/builtins-ppc-xlcompat-fp.ll
19

you can remove #0, #1 and #2

quinnp updated this revision to Diff 356264.Fri, Jul 2, 1:25 PM
quinnp marked an inline comment as done.

Addressing review comments.

quinnp updated this revision to Diff 356272.Fri, Jul 2, 2:01 PM
quinnp marked 4 inline comments as done.

Fixing a typo and the format of a test.

quinnp added inline comments.Tue, Jul 6, 10:45 AM
clang/test/CodeGen/builtins-ppc-xlcompat-sync.c
2 ↗(On Diff #352240)

This was to match the testing convention for XL compatibility builtins of targeting pwr7 for big endian and pwr8 for little endian. However, I will not be changing the sync tests in this patch, I have reversed those changes and will be putting them into a different patch.

amyk added a subscriber: amyk.Thu, Jul 8, 7:39 PM
amyk added inline comments.
clang/test/CodeGen/builtins-ppc-xlcompat-fp.c
4

nit: indentation on every second run line. We usually do two spaces more than the previous line. So I think it would look like:

20

Do the entry lines cause an issue when we don't have asserts?

llvm/include/llvm/IR/IntrinsicsPowerPC.td
1564

Minor indentation nit.

1567

Minor indentation nit to make it aligned with above.

llvm/test/CodeGen/builtins-ppc-xlcompat-fp.ll
3

Minor indentation nit:

quinnp updated this revision to Diff 357516.Fri, Jul 9, 8:15 AM

Adressing some review comments.

nemanjai added inline comments.Tue, Jul 13, 7:13 AM
clang/test/CodeGen/builtins-ppc-xlcompat-fp.c
6

AFAICT, nothing changes with Power8 so you might as well just have run lines for Power7 (AIX) and Power8 (LE).

llvm/include/llvm/IR/IntrinsicsPowerPC.td
1564

I don't think this indentation is desired. This makes it look like the first type on line 1529 belongs in the first list rather than the second list it belongs to. I suggest the following:

def int_ppc_fsel :
  GCCBuiltin<"__builtin_ppc_fsel">,
  Intrinsic<[llvm_double_ty], [llvm_double_ty,
                               llvm_double_ty, llvm_double_ty], [IntrNoMem]>;
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4986

I agree that this warrants a comment. However I don't think this is really what Victor had in mind. In general if your comment boils down to "this is what we are doing" and simply describes the code, it is not very useful. What your comment should address is "why are we doing this here and this way".

To me as a reader, it is not at all clear why we manually select this to PPC::FSELS here. All the other intrinsics are matched with patterns in the .td file but this one is matched specifically here. Why?

llvm/test/CodeGen/builtins-ppc-xlcompat-fp.ll
3

Again, this is excessive. There should be run lines for Power8, Power8-32-bit, Power8-no-vsx, Power7. We don't really need a cross-product of triples and CPUs.

nemanjai requested changes to this revision.Tue, Jul 13, 7:16 AM

Requesting changes to remove it from the queue until comments are addressed or answered.

This revision now requires changes to proceed.Tue, Jul 13, 7:16 AM
quinnp edited the summary of this revision. (Show Details)Fri, Jul 16, 6:28 AM
quinnp updated this revision to Diff 359426.Fri, Jul 16, 1:15 PM
quinnp marked 8 inline comments as done.

Addressing some review comments.

quinnp updated this revision to Diff 359437.Fri, Jul 16, 1:49 PM

Adding a better comment for the handling of the ppc_fsels intrinsic.

quinnp marked an inline comment as done.Fri, Jul 16, 1:49 PM
quinnp updated this revision to Diff 359441.Fri, Jul 16, 2:01 PM

Fixing a run line in a test.

nemanjai accepted this revision.Sun, Jul 18, 2:53 PM

LGTM aside from a very minor nit.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4986

s/ppc_fels intrinsic/PPC::FSELS instruction

since we aren't emitting an intrinsic, we are consuming it and emitting the instruction (well a MachineSDNode, but it represents an instruction).

This revision is now accepted and ready to land.Sun, Jul 18, 2:53 PM
amyk accepted this revision.Mon, Jul 19, 6:02 AM

My comments were addressed and I agree with the new indentation - LGTM.

NeHuang accepted this revision.Mon, Jul 19, 6:32 AM

LGTM

quinnp updated this revision to Diff 359783.Mon, Jul 19, 7:21 AM

Addressing review comment.

quinnp marked an inline comment as done.Mon, Jul 19, 7:22 AM
lei added a subscriber: lei.Tue, Jul 20, 6:52 AM

please rebase to ToT

quinnp updated this revision to Diff 360128.Tue, Jul 20, 7:40 AM

Rebasing patch. Moving macro definitions from a header file to a src file.

quinnp updated this revision to Diff 360261.Tue, Jul 20, 1:57 PM

Rebasing to ToT

quinnp updated this revision to Diff 360408.Wed, Jul 21, 5:47 AM

Rebasing to ToT.

This revision was landed with ongoing or failed builds.Wed, Jul 21, 6:33 AM
This revision was automatically updated to reflect the committed changes.

You missed a REQUIRES for the llvm test, I added one in: https://reviews.llvm.org/rG2404834c206a8930b0c420d94f4941b31c355de5

So if you see Arm-AArch64 quick bot failures, that was the reason.

You missed a REQUIRES for the llvm test, I added one in: https://reviews.llvm.org/rG2404834c206a8930b0c420d94f4941b31c355de5

So if you see Arm-AArch64 quick bot failures, that was the reason.

Right, sorry about that. Thank you, I will also move the test to llvm/test/CodeGen/PowerPC