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

[Anastasia Stulova via Phabricator via llvm-commits]

Anastasia added a comment.

Generally LGTM!

I only have one suggestion regarding testing - I feel that it makes more sense to just omit the tests that are not specific to the target itself. This is mainly because SPIR-V is now inherited from SPIR so the same logic applies in most of places. I think it would make sense that we only add tests for the logic which is not identical to SPIR. For example, tests that verify the target properties and target specific macros, etc make sense to keep but most of the tests that are just checking the language semantic using SPIR triple are not going to be different for SPIR-V (at least not now). So we can slowly build up relevant testing in the subsequent patches and therefore avoid unnessasary test time increase.



================
Comment at: clang/lib/Basic/Targets.cpp:609
   }
+  case llvm::Triple::spirv32: {
+    if (os != llvm::Triple::UnknownOS ||
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109144