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

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

Keenuts added a comment.

Thanks for the review. Addressed the feedback.  Waiting on https://reviews.llvm.org/D155978 to be merged before rebasing.



================
Comment at: llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp:539
+
+  report_fatal_error("Unimplemented environment for SPIR-V generation.");
+}
----------------
bogner wrote:
> 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.
Yes, it is a behavior change, but was unreachable. (`isOpenCLEnv()` was as you said, always `true`).
Now that the value can be something else, I thought a noisy failure if we reached an in-between state was probably better.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp:925
+          SPIRV::ExecutionMode::LocalSize, ST);
+    }
     if (F.getMetadata("work_group_size_hint"))
----------------
bogner wrote:
> 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.
Good catch.
Right now, we emit both, as this is valid SPIR-V, but the order depends of the order of those conditions, which makes is I believe not useful at all, just weird.
Adding an error in such cases, as I don't see a valid use-case for this.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp:47
+  //  (PhysicalStorageBuffer64).
+  return 32;
 }
----------------
bogner wrote:
> 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.
Yes, that's better. Removed this in favor of getting it from the DataLayout (via TargetMachine)


================
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-"
----------------
bogner wrote:
> Probably need a TODO here instead of or in addition to the one in `SPIRVSubtarget.cpp`'s `computePointerSize`
Done. Removed `computePointerSize` since we get it from datalayout, and moved the TODO here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156424