[PATCH] D116463: [SPIRV 4/6] Add target lowerling, TargetMachine and AsmPrinter

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp:77
+
+bool SPIRVSubtarget::isKernel() const {
+  return UsesOpenCLEnv || !UsesLogicalAddressing;
----------------
This doesn't look like a subtarget property, and also depending on the language seems conceptually problematic


================
Comment at: llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp:82
+bool SPIRVSubtarget::isShader() const {
+  return UsesVulkanEnv || UsesLogicalAddressing;
+}
----------------
Ditto 


================
Comment at: llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp:87
+bool SPIRVSubtarget::canDirectlyComparePointers() const {
+  bool Res = isAtLeastVer(TargetSPIRVVersion, 14);
+  return Res;
----------------
MaskRay wrote:
> remove used-once variable
The SPIR version seems like it should be a target machine level version, not subtarget property


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

https://reviews.llvm.org/D116463