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

[Paulo Matos via Phabricator via cfe-commits]

pmatos added inline comments.


================
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
----------------
Keenuts wrote:
> 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.
I know what you did and it makes sense - I did the same myself while working on another backend but lets try not to change such a large amount of lines in a patch focused on something else. I think it's certainly better to propose this change in a single NFC patch if we think that clang-format output is better than the existing formatting.


================
Comment at: llvm/unittests/TargetParser/TripleTest.cpp:1307
+  EXPECT_TRUE(T.isSPIRV());
+
   T.setArch(Triple::spirv32);
----------------
Keenuts wrote:
> 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.
> 
> 
This is a tricky one tbh - I would have to do a bit more research to have a more insighful reply here. Maybe @mpaszkowski or @Anastasia can add more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155978