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

[Paulo Matos via Phabricator via cfe-commits]

pmatos added subscribers: iliya-diyachkov, zuban32.
pmatos added a comment.

Thanks for the patch. Left a few comments. Others can take a better look, I am sure. Maybe @iliya-diyachkov or @zuban32 can take a look as well.



================
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
----------------
No need to reindent the whole block to add a single line.


================
Comment at: llvm/lib/TargetParser/Triple.cpp:74
+  case spirv:
+    return "spirv";
   case spirv32:        return "spirv32";
----------------
This should be in the same line.


================
Comment at: llvm/lib/TargetParser/Triple.cpp:387
+      .Case("spir64", spir64)
+      .Case("spirv", spirv)
+      .Case("spirv32", spirv32)
----------------
Same as above. I guess there's no need to reindent a whole block.


================
Comment at: llvm/lib/TargetParser/Triple.cpp:529
+          .Case("spir64", Triple::spir64)
+          .Cases("spirv", "spirv1.0", "spirv1.1", "spirv1.2", "spirv1.3",
+                 "spirv1.4", "spirv1.5", Triple::spirv)
----------------
Whole block reindent.


================
Comment at: llvm/unittests/TargetParser/TripleTest.cpp:1307
+  EXPECT_TRUE(T.isSPIRV());
+
   T.setArch(Triple::spirv32);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155978