[PATCH] D109144: [SPIR-V] Add SPIR-V triple architecture and clang target info

[Henry Linjamäki via Phabricator via llvm-commits]

linjamaki marked 2 inline comments as done.
linjamaki added inline comments.


================
Comment at: clang/lib/Basic/Targets.cpp:609
   }
+  case llvm::Triple::spirv32: {
+    if (os != llvm::Triple::UnknownOS ||
----------------
Anastasia wrote:
> I wonder how complete is the support of logical addressing SPIR-V triple? It seems like you don't test it in the clang invocation at the moment and it is therefore missing from `TargetInfo`. 
> 
> Do you have plans to implement it in the subsequent patches? If not it might be better to leave it out for now.
I removed the spirvlogical triple. AFAIK, this triple has zero sized pointers and I don't know if the LLVM is ready for that yet.


================
Comment at: clang/test/CodeGenOpenCL/spirv32_target.cl:12
+kernel void foo(global long *arg) {
+  int res1[sizeof(my_st)  == 12 ? 1 : -1];
+  int res2[sizeof(void *) ==  4 ? 1 : -1];
----------------
Anastasia wrote:
> Are these lines tested somehow? You could change this into C++ for OpenCL test and use `static_assert` or find some other ways to test this...
> 
> However, this testing seems to overlap with the lines at the end... Could you please elaborate on the intent of this test?
> 
> Also if you don't plan this to be fundamentally different from testing of 64bit triple I think this should be merged with `spirv64_target.cl`. It will make things easier for the maintenance and further evolution.
I used spir{32,64}_target.cl as a base for checking codegen for SPIR-V with the only difference being the triple check. The lines give an compile error (e.g. error: 'res1' declared as an array with a negative size) if the sizeof operators give unexpected result. 

I'll merge this file with the spirv64_target.cl.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109144