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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
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];
----------------
linjamaki wrote:
> 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.
> 
Ok, you could consider adding `-verify` and 

`//expected-no-diagnostics`

Even though FileCheck should not succeed but it is going to be more clear that some functionality is tested via diagnostics for the future modifications. 

However, you seem to be testing pointer size in the two lines below which also seem to be tested via `//CHECK` directives... not sure whether both are needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109144