[PATCH] D156424: [SPIRV] Add SPIR-V logical triple to llc

[Justin Bogner via Phabricator via llvm-commits]

bogner accepted this revision.
bogner added a comment.
This revision is now accepted and ready to land.

This looks like a good start. I think a fair amount of it will need to change as the HLSL support matures, but most of that should be fine to handle in tree.



================
Comment at: llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp:436
+    Inst.addOperand(MCOperand::createImm(V));
+  }
+
----------------
Not a problem with your patch in particular, but we need to change how we represent numthreads to something more sensible. Having every user of it parsing this string is ridiculous.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:249
+  report_fatal_error("This HLSL entrypoint is not supported by this backend.");
+}
+
----------------
These both look reasonable for now, though as you mention we need to discuss unifying how we represent this stuff


================
Comment at: llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp:539
+
+  report_fatal_error("Unimplemented environment for SPIR-V generation.");
+}
----------------
Is this is a behaviour change? Looks like we'd just carry on without any capabilities before, but `isOpenCLEnv()` always returned true so maybe it was just impossible.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp:925
+          SPIRV::ExecutionMode::LocalSize, ST);
+    }
     if (F.getMetadata("work_group_size_hint"))
----------------
Does this do something reasonable if both "reqd_work_group_size" and "hlsl.numthreads" are set? By reasonable I mean something like "error" or "accept them if they're compatible", as opposed to "choose one arbitrarily". Might become irrelevant if we're able to unify the representation.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp:47
+  //  (PhysicalStorageBuffer64).
+  return 32;
 }
----------------
Can we get this from the datalayout at this point? It'd be nice to not have to update this in two places when we revisit it.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp:49
   const auto Arch = TT.getArch();
-  if (Arch == Triple::spirv32)
+  if (Arch == Triple::spirv32 || Arch == Triple::spirv)
     return "e-p:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-"
----------------
Probably need a TODO here instead of or in addition to the one in `SPIRVSubtarget.cpp`'s `computePointerSize`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156424