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

[Ilia Diachkov via Phabricator via llvm-commits]

iliya-diyachkov added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp:87
+bool SPIRVSubtarget::canDirectlyComparePointers() const {
+  bool Res = isAtLeastVer(TargetSPIRVVersion, 14);
+  return Res;
----------------
arsenm wrote:
> MaskRay wrote:
> > remove used-once variable
> The SPIR version seems like it should be a target machine level version, not subtarget property
@arsenm I guess you propose to make the SPIR-V version an analogue of Processor and represent the version's features as SubtargetFeatures. Then we fill each "Processor" (i.e. the SPIR-V version) with its features and the features become subtarget property.

It looks good, but as I see there is an alternative approach: NVPTX's subtarget contains SmVersion that is used to evaluate subtarget's features in methods of NVPTXSubtarget class. The SPIR-V implementation uses the similar manner defining TargetSPIRVVersion as a member of SPIRVSubtarget and evaluating subtarget's features using the version number. Not all the subtarget's methods (which use the version) are published in the patch series since they are connected to the parts that are not yet present in it.

Perhaps this is a reasonable argument for keeping TargetSPIRVVersion a member of Subtarget. Maybe we'd better change its name to shorter and less confusing e.g. "SPIRVVersion".

What about other version members (TargetOpenCLVersion, TargetVulkanVersion), we don't use them in this series so they may be currently omitted for now.


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

https://reviews.llvm.org/D116463