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.
Details
- Reviewers
nemanjai NeHuang - Group Reviewers
Restricted Project - Commits
- rGef49d925e2a7: [PowerPC] Implement trap and conversion builtins for XL compatibility
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
3346 | 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 ↗ | (On Diff #352431) | 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. |
clang/include/clang/Basic/BuiltinsPPC.def | ||
---|---|---|
32 | seems like rebase issue that comments got overwritten. | |
50 | definition here not matching prototype in document void __tdw ( long a, long b, unsigned int TO); | |
51 | prototype void __tw (int a, int b, unsigned int TO); why not using Ui for the last arg? | |
clang/lib/Sema/SemaChecking.cpp | ||
3347 | range suppose to be 0 to 31 based on document. | |
3349 | same as above. | |
clang/test/CodeGen/builtins-ppc-xlcompat-conversionfunc.c | ||
3 | are these builtins all pwr9 only?
| |
10 | you can define extern variables here for the bulitins. | |
clang/test/CodeGen/builtins-ppc-xlcompat-error.c | ||
17 | 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: | |
19 | same as above | |
clang/test/CodeGen/builtins-ppc-xlcompat-trap.c | ||
3 | same as above. | |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-conversionfunc.ll | ||
4 | 32 bit and 64 bit results look identical, you do not need prefixes. |
clang/test/CodeGen/builtins-ppc-xlcompat-error.c | ||
---|---|---|
12 | nit: Have these variables as extern like the other patches do. | |
clang/test/CodeGen/builtins-ppc-xlcompat-trap.c | ||
15 | nit: Have these variables as extern like the other patches do. | |
llvm/lib/Target/PowerPC/PPCInstrInfo.td | ||
2233 | Are tdne, tweq supposed to be aliases that are intended to be added here, too? | |
llvm/lib/Target/PowerPC/PPCInstrPrefix.td | ||
2840 ↗ | (On Diff #355130) | Question: Why are these in PPCInstrPrefix.td? Shouldn't they be in PPCInstrInfo.td instead? |
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.
clang/test/CodeGen/builtins-ppc-xlcompat-conversionfunc.c | ||
---|---|---|
3 | Please use pwr7 for BE test and pwr8 for LE test. | |
clang/test/CodeGen/builtins-ppc-xlcompat-trap-64bit-only.c | ||
9 | pwr8 -> pwr7 | |
12 | pwr8 -> pwr7 | |
clang/test/CodeGen/builtins-ppc-xlcompat-trap.c | ||
9 | same as above | |
12 | same as above. | |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-conversionfunc.ll | ||
8 | 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 | ||
29 | 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)>; | |
42 | seems the InstAlias defined for td and tw not working as expected |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap.ll | ||
---|---|---|
136 | Where are the aliases twnei and tdnei coming from? You don't seem to add them. |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap.ll | ||
---|---|---|
136 | it should be from the trap extend mnemonics |
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-64bit-only.ll | ||
---|---|---|
42 | after discussion with Nemanja, we'll leave out the inst alias as that is a very low priority fix. |
llvm/lib/Target/PowerPC/PPCInstr64Bit.td | ||
---|---|---|
1728 | This is supposed to be an unconditional trap and the produced sequence is not that. |
llvm/lib/Target/PowerPC/PPCInstr64Bit.td | ||
---|---|---|
1728 | this one is quite weird... I see the output on xlC as tdnei: 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); } |
seems like rebase issue that comments got overwritten.