[PATCH] D103668: [PowerPC] Implement trap and conversion builtins for XL compatibility

[Victor Huang via Phabricator via cfe-commits]

NeHuang requested changes to this revision.
NeHuang added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/include/clang/Basic/BuiltinsPPC.def:32
 
-// builtins for compatibility with the XL compiler
+// XL Compatibility built-ins
 BUILTIN(__builtin_ppc_popcntb, "ULiULi", "")
----------------
seems like rebase issue that comments got overwritten.


================
Comment at: clang/include/clang/Basic/BuiltinsPPC.def:50
 BUILTIN(__builtin_ppc_compare_and_swaplp, "iLiD*Li*Li", "")
+BUILTIN(__builtin_ppc_tdw, "vLLiLLiIi", "")
+BUILTIN(__builtin_ppc_tw, "viiIi", "")
----------------
definition here not matching prototype in document 
```
void __tdw ( long a, long b, unsigned int TO);
```


================
Comment at: clang/include/clang/Basic/BuiltinsPPC.def:51
+BUILTIN(__builtin_ppc_tdw, "vLLiLLiIi", "")
+BUILTIN(__builtin_ppc_tw, "viiIi", "")
+BUILTIN(__builtin_ppc_trap, "vi", "")
----------------
prototype 
```
void __tw (int a, int b, unsigned int TO);
```
why not using `Ui` for the last arg?  


================
Comment at: clang/lib/Sema/SemaChecking.cpp:3347
+  case PPC::BI__builtin_ppc_tw:
+    return SemaBuiltinConstantArgRange(TheCall, 2, 1, 31);
+  case PPC::BI__builtin_ppc_tdw:
----------------
range suppose to be 0 to 31 based on document.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:3349
+  case PPC::BI__builtin_ppc_tdw:
+    return SemaBuiltinConstantArgRange(TheCall, 2, 1, 31);
 #define CUSTOM_BUILTIN(Name, Intr, Types, Acc) \
----------------
same as above.


================
Comment at: clang/test/CodeGen/builtins-ppc-xlcompat-conversionfunc.c:2
+// RUN: %clang_cc1 -O2 -triple powerpc64-unknown-unknown \
+// RUN:   -emit-llvm %s -o - -target-cpu pwr9 | FileCheck %s
+// RUN: %clang_cc1 -O2 -triple powerpc64le-unknown-unknown \
----------------
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.


================
Comment at: clang/test/CodeGen/builtins-ppc-xlcompat-conversionfunc.c:9
+// RUN:   -emit-llvm %s -o - -target-cpu pwr9 | FileCheck %s
+
+double test_fcfid(double a) {
----------------
you can define extern variables here for the bulitins.


================
Comment at: clang/test/CodeGen/builtins-ppc-xlcompat-error.c:16
+#ifdef __PPC64__
+  __tdw(lla, llb, 50); //expected-error {{argument value 50 is outside the valid range [1, 31]}}
+#endif
----------------
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:
```


================
Comment at: clang/test/CodeGen/builtins-ppc-xlcompat-error.c:18
+#endif
+  __tw(ia, ib, 50); //expected-error {{argument value 50 is outside the valid range [1, 31]}}
+}
----------------
same as above 


================
Comment at: clang/test/CodeGen/builtins-ppc-xlcompat-trap.c:2
+// RUN: %clang_cc1 -O2 -triple powerpc64-unknown-unknown \
+// RUN:   -emit-llvm %s -o - -target-cpu pwr9 | \
+// RUN:  FileCheck %s --check-prefixes=CHECK64,CHECK
----------------
same as above.


================
Comment at: llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-conversionfunc.ll:3
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu \
+; RUN:   -mcpu=pwr9 < %s | FileCheck %s --check-prefix=CHECK-64
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu \
----------------
32 bit and 64 bit results look identical, you do not need prefixes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103668/new/

https://reviews.llvm.org/D103668