Page MenuHomePhabricator

[PowerPC] Implement trap and conversion builtins for XL compatibility
ClosedPublic

Authored by Conanap on Jun 3 2021, 10:31 PM.

Details

Summary

This patch implements trap and FP to and from double conversions. The builtins
generate code that mirror what is generated from the XL compiler. Intrinsics
are named conventionally with builtin_ppc, but are aliased to provide the same
builtin names as the XL compiler.

Diff Detail

Unit TestsFailed

TimeTest
80 msx64 debian > LLVM.CodeGen/PowerPC::builtins-ppc-xlcompat-conversionfunc.c
Script: -- : 'RUN: at line 1'; clang -mcpu=pwr9 -O2 -c -S /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-conversionfunc.c -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-conversionfunc.c
60 msx64 debian > LLVM.CodeGen/PowerPC::builtins-ppc-xlcompat-trap.c
Script: -- : 'RUN: at line 1'; clang -mcpu=pwr7 -m64 -O2 -c -S /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap.c -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap.c
160 msx64 windows > LLVM.CodeGen/PowerPC::builtins-ppc-xlcompat-conversionfunc.c
Script: -- : 'RUN: at line 1'; clang -mcpu=pwr9 -O2 -c -S C:\ws\w6\llvm-project\premerge-checks\llvm\test\CodeGen\PowerPC\builtins-ppc-xlcompat-conversionfunc.c -o - | c:\ws\w6\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w6\llvm-project\premerge-checks\llvm\test\CodeGen\PowerPC\builtins-ppc-xlcompat-conversionfunc.c
130 msx64 windows > LLVM.CodeGen/PowerPC::builtins-ppc-xlcompat-trap.c
Script: -- : 'RUN: at line 1'; clang -mcpu=pwr7 -m64 -O2 -c -S C:\ws\w6\llvm-project\premerge-checks\llvm\test\CodeGen\PowerPC\builtins-ppc-xlcompat-trap.c -o - | c:\ws\w6\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w6\llvm-project\premerge-checks\llvm\test\CodeGen\PowerPC\builtins-ppc-xlcompat-trap.c

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Conanap updated this revision to Diff 352431.Jun 16 2021, 7:29 AM

Rebase and added 64 bit sema checking

Rebased to include the updated location for the builtin alias
definition location, as well as added 64 bit checking for 64
bit only builtins in sema checking.

qiucf added a subscriber: qiucf.Jun 16 2021, 11:28 PM
qiucf added inline comments.
clang/lib/Sema/SemaChecking.cpp
3334

Unnecessary brace?

llvm/include/llvm/IR/IntrinsicsPowerPC.td
115

This comment is little bit confusing: we don't have __builtin_ppc_fcfid in XL. XL has __fcfid instead. What we're doing is to follow Clang's convention to name them __builtin_xxx and defines macros to maintain XL compatibility.

llvm/lib/Target/PowerPC/PPCInstr64Bit.td
1735

Where is trap?

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-conversionfunc.c
1

Please separate front-end and back-end tests. IIRC, LLVM doesn't accept end-to-end tests yet. So it's better to put 'C to IR' test in Clang's codegen, and put 'IR to Assembly' test in llvm/PowerPC's codegen.

Use %clang_cc1 in front-end tests. And also test in back-end if VSX is not available.

Conanap updated this revision to Diff 355129.Jun 29 2021, 12:47 AM

Separated new test cases, rebased for newest changes, different semachecking

Conanap updated this revision to Diff 355130.Jun 29 2021, 12:50 AM
Conanap marked 3 inline comments as done.

Removed unneccessary brackets

Addressed comments

NeHuang requested changes to this revision.Jun 30 2021, 11:48 AM
NeHuang added a subscriber: NeHuang.
NeHuang added inline comments.
clang/include/clang/Basic/BuiltinsPPC.def
32

seems like rebase issue that comments got overwritten.

48

definition here not matching prototype in document

void __tdw ( long a, long b, unsigned int TO);
49

prototype

void __tw (int a, int b, unsigned int TO);

why not using Ui for the last arg?

clang/lib/Sema/SemaChecking.cpp
3335

range suppose to be 0 to 31 based on document.

3337

same as above.

clang/test/CodeGen/builtins-ppc-xlcompat-conversionfunc.c
2 ↗(On Diff #355130)

are these builtins all pwr9 only?

  • If yes, please rename the file.
  • If not, please use pwr8 for LE test and pwr7 for BE cases.
9 ↗(On Diff #355130)

you can define extern variables here for the bulitins.

clang/test/CodeGen/builtins-ppc-xlcompat-error.c
16 ↗(On Diff #355130)

range should be 0 to 31 as described in document.

TO A value of 0 to 31 inclusive. Each bit position, if set, indicates one or more of
the following possible conditions:
18 ↗(On Diff #355130)

same as above

clang/test/CodeGen/builtins-ppc-xlcompat-trap.c
2 ↗(On Diff #355130)

same as above.

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-conversionfunc.ll
3 ↗(On Diff #355130)

32 bit and 64 bit results look identical, you do not need prefixes.

This revision now requires changes to proceed.Jun 30 2021, 11:48 AM
amyk added a subscriber: amyk.Jul 5 2021, 6:42 AM
amyk added inline comments.
clang/test/CodeGen/builtins-ppc-xlcompat-error.c
11 ↗(On Diff #355130)

nit: Have these variables as extern like the other patches do.

clang/test/CodeGen/builtins-ppc-xlcompat-trap.c
14 ↗(On Diff #355130)

nit: Have these variables as extern like the other patches do.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
2233 ↗(On Diff #355130)

Are tdne, tweq supposed to be aliases that are intended to be added here, too?

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
2840

Question: Why are these in PPCInstrPrefix.td? Shouldn't they be in PPCInstrInfo.td instead?

Conanap updated this revision to Diff 356549.Jul 5 2021, 12:07 PM

Changed to ForceXForm

Conanap updated this revision to Diff 356550.Jul 5 2021, 12:09 PM

Changed to ForceXForm

Conanap updated this revision to Diff 356552.Jul 5 2021, 12:14 PM

Fixed incorrect diff update

nemanjai requested changes to this revision.Jul 7 2021, 3:16 AM

It appears that Victor's comments have not been addressed yet and this is not ready for the next round of review. Requesting changes again to take it out of the queue until Victor's comments are addressed.

This revision now requires changes to proceed.Jul 7 2021, 3:16 AM
Conanap updated this revision to Diff 356985.Jul 7 2021, 9:53 AM
Conanap marked 15 inline comments as done.

Addressed comments, separated 64 bit C test cases.

clang/include/clang/Basic/BuiltinsPPC.def
48

the document had been incorrect; the document is now updated to to long long.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
2233 ↗(On Diff #355130)

the compiler seem to already know tdne and tweq so I didn't add those.

Conanap updated this revision to Diff 356988.Jul 7 2021, 10:11 AM

Moved inst alias for 64bit to the 64 bit file

NeHuang resigned from this revision.Jul 7 2021, 11:40 AM
NeHuang added inline comments.
clang/test/CodeGen/builtins-ppc-xlcompat-conversionfunc.c
2 ↗(On Diff #356988)

Please use pwr7 for BE test and pwr8 for LE test.

clang/test/CodeGen/builtins-ppc-xlcompat-trap-64bit-only.c
8 ↗(On Diff #356988)

pwr8 -> pwr7

11 ↗(On Diff #356988)

pwr8 -> pwr7

clang/test/CodeGen/builtins-ppc-xlcompat-trap.c
8 ↗(On Diff #356988)

same as above

11 ↗(On Diff #356988)

same as above.

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-conversionfunc.ll
7 ↗(On Diff #356988)

same target cpu issue for the aix run lines in back end test cases.

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-64bit-only.ll
28 ↗(On Diff #356988)

can you add another test case for call void @llvm.ppc.tdw(i64 %a, i64 %b, i32 3) to verify the backend change below:

// tdne
def : Pat<(int_ppc_tdw g8rc:$A, g8rc:$B, 3),
          (TD 24, $A, $B)>;
41 ↗(On Diff #356988)

seems the InstAlias defined for td and tw not working as expected

NeHuang requested changes to this revision.Jul 7 2021, 11:42 AM
This revision now requires changes to proceed.Jul 7 2021, 11:42 AM
nemanjai added inline comments.Jul 7 2021, 12:03 PM
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap.ll
135 ↗(On Diff #356988)

Where are the aliases twnei and tdnei coming from? You don't seem to add them.

Conanap updated this revision to Diff 357052.Jul 7 2021, 12:39 PM
Conanap marked 6 inline comments as done.

Rebased and changed aix test cases to pwr 7

Conanap marked an inline comment as done.Jul 7 2021, 12:56 PM
Conanap added inline comments.
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap.ll
135 ↗(On Diff #356988)

it should be from the trap extend mnemonics

Conanap marked an inline comment as done.Jul 7 2021, 8:23 PM
Conanap updated this revision to Diff 357127.Jul 7 2021, 8:40 PM

Removed inst aliases

Conanap added inline comments.Jul 8 2021, 7:02 AM
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-64bit-only.ll
41 ↗(On Diff #356988)

after discussion with Nemanja, we'll leave out the inst alias as that is a very low priority fix.

Conanap updated this revision to Diff 357270.Jul 8 2021, 10:28 AM

Removed some incorrect patterns

nemanjai requested changes to this revision.Jul 8 2021, 10:30 AM
nemanjai added inline comments.
llvm/lib/Target/PowerPC/PPCInstr64Bit.td
1728

This is supposed to be an unconditional trap and the produced sequence is not that.

This revision now requires changes to proceed.Jul 8 2021, 10:30 AM
Conanap marked an inline comment as done.Jul 8 2021, 11:54 AM
Conanap added inline comments.
llvm/lib/Target/PowerPC/PPCInstr64Bit.td
1728

this one is quite weird... I see the output on xlC as tdnei:
0b 04 00 00 tdnei r4,0

Also for the tdne pattern that I removed (TD 3, ...), xlC outputs the same encoding for TD 3 and TD 24:

Disassembly of section .text:

0000000000000000 <.yes>:
   0:   7f 03 20 88     tdne    r3,r4
   4:   4e 80 00 20     blr
   8:   00 00 00 00     .long 0x0
   c:   00 00 20 00     .long 0x2000
  10:   00 00 00 00     .long 0x0
  14:   00 00 00 08     .long 0x8
        ...
void yes(long long a, long long b) {
   return __tdw(a, b, 24);
}

and for 3:

Disassembly of section .text:

0000000000000000 <.yes>:
   0:   7f 03 20 88     tdne    r3,r4
   4:   4e 80 00 20     blr
   8:   00 00 00 00     .long 0x0
   c:   00 00 20 00     .long 0x2000
  10:   00 00 00 00     .long 0x0
  14:   00 00 00 08     .long 0x8
        ...
void yes(long long a, long long b) {
   return __tdw(a, b, 3);
}
Conanap updated this revision to Diff 357334.Jul 8 2021, 1:06 PM

Removed TWNE pattern as well

nemanjai accepted this revision.Jul 8 2021, 1:23 PM

LGTM. @NeHuang please have a look to see if your comments are adequately addressed.

NeHuang accepted this revision.Jul 8 2021, 1:36 PM

LGTM. Thanks for addressing the comments!

This revision is now accepted and ready to land.Jul 8 2021, 1:36 PM