[PATCH] D155978: [SPIRV] Add SPIR-V logical triple.

[Nathan Gauër via Phabricator via llvm-commits]

Keenuts marked 4 inline comments as done.
Keenuts added a comment.

Thanks for the review 😊
Reverted clang-format patch, and answered inline.



================
Comment at: llvm/include/llvm/TargetParser/Triple.h:106
+    wasm32,      // WebAssembly with 32-bit pointers
+    wasm64,      // WebAssembly with 64-bit pointers
     renderscript32, // 32-bit RenderScript
----------------
pmatos wrote:
> No need to reindent the whole block to add a single line.
IIRC it I did a clang-format because the buildbot complained the format was not right.
Reverted the clang-format commit (which seems better, I agree), and I'll see if the bots complains.


================
Comment at: llvm/unittests/TargetParser/TripleTest.cpp:1307
+  EXPECT_TRUE(T.isSPIRV());
+
   T.setArch(Triple::spirv32);
----------------
pmatos wrote:
> I am not well-versed in SPIRV for gfx but if we are using 32bits in SPIRV logical, can't we reuse spirv32? Is there some sort of strong reason not to or adding spirv for logical spirv as an alternative to spirv32 and spirv64 is just easier?
This is a very valid question! And I'm not sure of the best option.
My understanding is: in logical SPIR-V, there are no notion of pointer size, or architecture size. We cannot offset pointers by a sizeof(), or do such things.

But the options I had didn't seem to allow this kind of "undefined architecture".
I chose 32bits here because the IDs are 32bit. But pointer addresses are not, so returning 32bit is not quite right either.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155978