[HIP] emit macro `__HIP_NO_IMAGE_SUPPORT`

[yaxunl]

Summary

HIP texture/image support is optional as some devices
do not have image instructions. A macro __HIP_NO_IMAGE_SUPPORT
is defined for device not supporting images (https://github.com/ROCm-Developer-Tools/HIP/blob/d0448aa4c4dd0f4b29ccf6a663b7f5ad9f5183e0/docs/reference/kernel_language.md?plain=1#L426 )

Currently the macro is defined by HIP header based on predefined macros
for GPU, e.g __gfx*__ , which is error prone. This patch let clang
emit the predefined macro.

[yaxunl]

update tests

[arsenm]

You seem to be defining a new subtarget feature without actually defining the underlying feature. I thought the issue was specific missing image instructions, which is already covered by extended-image-insts

clang/lib/Basic/Targets/AMDGPU.h

458

A negative form shouldn’t apply to targets with images, it will break the IR.

[arsenm]

clang/test/OpenMP/amdgcn-attributes.cpp

36 ↗

(On Diff #525637)

There’s also no reason to emit these in the IR. Really we should stop emitting most of these

[yaxunl]

In D151349#4372695, @arsenm wrote:

You seem to be defining a new subtarget feature without actually defining the underlying feature. I thought the issue was specific missing image instructions, which is already covered by extended-image-insts

gfx90a does not have extended-image-insts but supports image/texture API's in HIP. Brian suggested to determine whether HIP image is supported based on whether the subtarget has image-insts.

I thought image-insts is a subtarget feature as it is defined in https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/AMDGPU.td#L499

Did I miss something?

[yaxunl]

clang/test/OpenMP/amdgcn-attributes.cpp

36 ↗

(On Diff #525637)

I agree.

For example, for image-insts, we do not need to expose it to FE since there is no point of enable/disable it in FE. We only need to know whether the GPU supports image insts or not. In this case, it seems simpler to just get that information through ISA version.

[yaxunl]

using ISA version to determine whether image is supported

[tra]

clang/lib/Basic/Targets/AMDGPU.cpp

248

My usual nit about negations: !(ISAVer.Major == 9 && ISAVer.Minor == 4) is easier to read.

Is ISA 9.4 the only version w/o image support? Or should it be a range comparison instead?

[arsenm]

In D151349#4377792, @yaxunl wrote:

using ISA version to determine whether image is supported

That’s backward. You can track the feature in clang separately and just not emit it. We do that for a few other features. You just stop the TargetParser parts

[yaxunl]

In D151349#4381471, @arsenm wrote:

In D151349#4377792, @yaxunl wrote:

using ISA version to determine whether image is supported

That’s backward. You can track the feature in clang separately and just not emit it. We do that for a few other features. You just stop the TargetParser parts

Sorry, I did not quite get it. Do you mean to add a feature to https://github.com/llvm/llvm-project/blob/85670ac86813b170c9301aa477421c56a71a7e1e/llvm/lib/TargetParser/TargetParser.cpp#L107 ? should I add FEATURE_IMAGE or FEATURE_NO_IMAGE ?

[arsenm]

In D151349#4381569, @yaxunl wrote:

In D151349#4381471, @arsenm wrote:

In D151349#4377792, @yaxunl wrote:

using ISA version to determine whether image is supported

That’s backward. You can track the feature in clang separately and just not emit it. We do that for a few other features. You just stop the TargetParser parts

Sorry, I did not quite get it. Do you mean to add a feature to https://github.com/llvm/llvm-project/blob/85670ac86813b170c9301aa477421c56a71a7e1e/llvm/lib/TargetParser/TargetParser.cpp#L107 ? should I add FEATURE_IMAGE or FEATURE_NO_IMAGE ?

It's expressed as positive image feature. I thought there was a feature where clang tracks it per-target, but doesn't emit it in the IR in "target-features" but I can't remember which one it is. Is there a way to track the image feature without emitting it?

[yaxunl]

In D151349#4395312, @arsenm wrote:

In D151349#4381569, @yaxunl wrote:

In D151349#4381471, @arsenm wrote:

In D151349#4377792, @yaxunl wrote:

using ISA version to determine whether image is supported

That’s backward. You can track the feature in clang separately and just not emit it. We do that for a few other features. You just stop the TargetParser parts

Sorry, I did not quite get it. Do you mean to add a feature to https://github.com/llvm/llvm-project/blob/85670ac86813b170c9301aa477421c56a71a7e1e/llvm/lib/TargetParser/TargetParser.cpp#L107 ? should I add FEATURE_IMAGE or FEATURE_NO_IMAGE ?

It's expressed as positive image feature. I thought there was a feature where clang tracks it per-target, but doesn't emit it in the IR in "target-features" but I can't remember which one it is. Is there a way to track the image feature without emitting it?

clang tracks target feature by TargetOptions::FeatureMap ( https://github.com/llvm/llvm-project/blob/7a2b12b05b39bce08987968270b43ca05d9a4836/clang/include/clang/Basic/TargetOptions.h#L62 )

Each target-specific TargetInfo-derived class initializes this map and CodeGen uses it to emit function attributes consistently.

There is no separate data structure to track target features not emitted to function attributes.

If we want to make these target features unchangeable and not emitted to IR, we could add a set for unchangeable target features and each target can initialize it. These features are read-only and will not be emitted to IR, and will cause a warning and be ignored if users try to force enabling or disabling them.

[yaxunl]

warn if users try to enable/disable image-insts and do not emit image-insts as function attributes in IR

[jhuber6]

I've started seeing these errors when compiling for OpenMP targeting AMDGPU:

$ clang input.c -fopenmp --offload-arch=gfx1030
warning: feature flag '+image-insts' is ignored since the feature is read only [-Winvalid-command-line-argument]

Any suggestions for what might cause this?

[yaxunl]

In D151349#4426008, @jhuber6 wrote:

I've started seeing these errors when compiling for OpenMP targeting AMDGPU:

$ clang input.c -fopenmp --offload-arch=gfx1030
warning: feature flag '+image-insts' is ignored since the feature is read only [-Winvalid-command-line-argument]

Any suggestions for what might cause this?

That means TargetInfo::initFeatureMap is called with +image-insts. I will take a look.

clang/include/clang/Basic/TargetInfo.h

#include "clang/Basic/OpenCLExtensions.def"
   /// Returns the version of the darwin target variant SDK which was used during
   /// the compilation if one was specified, or an empty version otherwise.
   const std::optional<VersionTuple> getDarwinTargetVariantSDKVersion() const {
     return !getTargetOpts().DarwinTargetVariantSDKVersion.empty()
                ? getTargetOpts().DarwinTargetVariantSDKVersion
                : std::optional<VersionTuple>();
   }
 
+  /// Whether to support HIP image/texture API's.
+  virtual bool hasHIPImageSupport() const { return true; }
+
 protected:
   /// Copy type and layout related info.
   void copyAuxTarget(const TargetInfo *Aux);
   virtual uint64_t getPointerWidthV(LangAS AddrSpace) const {
     return PointerWidth;
   }
   virtual uint64_t getPointerAlignV(LangAS AddrSpace) const {
     return PointerAlign;

clang/lib/Basic/Targets/AMDGPU.h

class LLVM_LIBRARY_VISIBILITY AMDGPUTargetInfo final : public TargetInfo {
 
   llvm::AMDGPU::GPUKind GPUKind;
   unsigned GPUFeatures;
   unsigned WavefrontSize;
 
   /// Whether to use cumode or WGP mode. True for cumode. False for WGP mode.
   bool CUMode;
 
+  /// Whether having image instructions.
+  bool HasImage;
+
   /// Target ID is device name followed by optional feature name postfixed
   /// by plus or minus sign delimitted by colon, e.g. gfx908:xnack+:sramecc-.
   /// If the target ID contains feature+, map it to true.
   /// If the target ID contains feature-, map it to false.
   /// If the target ID does not contain a feature (default), do not map it.
   llvm::StringMap<bool> OffloadArchFeatures;
   std::string TargetID;
 
for (const auto &F : Features) {
       if (F == "+wavefrontsize64")
         WavefrontSize = 64;
       else if (F == "+cumode")
         CUMode = true;
       else if (F == "-cumode")
         CUMode = false;
       bool IsOn = F.front() == '+';
       StringRef Name = StringRef(F).drop_front();
       if (!llvm::is_contained(TargetIDFeatures, Name))
         continue;
       assert(!OffloadArchFeatures.contains(Name));
       OffloadArchFeatures[Name] = IsOn;
     }
     return true;
   }
 
   std::optional<std::string> getTargetID() const override {
     if (!isAMDGCN(getTriple()))
       return std::nullopt;
     // When -target-cpu is not set, we assume generic code that it is valid
     // for all GPU and use an empty string as target ID to represent that.
     if (GPUKind == llvm::AMDGPU::GK_NONE)
       return std::string("");
     return getCanonicalTargetID(getArchNameAMDGCN(GPUKind),
                                 OffloadArchFeatures);
   }
+
+  bool hasHIPImageSupport() const override { return HasImage; }
 };
 
 } // namespace targets
 } // namespace clang
 
 #endif // LLVM_CLANG_LIB_BASIC_TARGETS_AMDGPU_H

clang/lib/Basic/Targets/AMDGPU.cpp

if (getMaxPointerWidth() == 64) {
     LongWidth = LongAlign = 64;
     SizeType = UnsignedLong;
     PtrDiffType = SignedLong;
     IntPtrType = SignedLong;
   }
 
   MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
   CUMode = !(GPUFeatures & llvm::AMDGPU::FEATURE_WGP);
+  auto ISAVer = llvm::AMDGPU::getIsaVersion(Opts.CPU);
+  HasImage = ISAVer.Major != 9 || ISAVer.Minor != 4;
 }
 
 void AMDGPUTargetInfo::adjust(DiagnosticsEngine &Diags, LangOptions &Opts) {
   TargetInfo::adjust(Diags, Opts);
   // ToDo: There are still a few places using default address space as private
   // address space in OpenCL, which needs to be cleaned up, then Opts.OpenCL
   // can be removed from the following line.
   setAddressSpaceMap(/*DefaultIsPrivate=*/Opts.OpenCL ||

clang/lib/Frontend/InitPreprocessor.cpp

static void InitializeStandardPredefinedMacros(const TargetInfo &TI,
   if (LangOpts.HIP) {
     Builder.defineMacro("__HIP__");
     Builder.defineMacro("__HIPCC__");
     Builder.defineMacro("__HIP_MEMORY_SCOPE_SINGLETHREAD", "1");
     Builder.defineMacro("__HIP_MEMORY_SCOPE_WAVEFRONT", "2");
     Builder.defineMacro("__HIP_MEMORY_SCOPE_WORKGROUP", "3");
     Builder.defineMacro("__HIP_MEMORY_SCOPE_AGENT", "4");
     Builder.defineMacro("__HIP_MEMORY_SCOPE_SYSTEM", "5");
-    if (LangOpts.CUDAIsDevice)
+    if (LangOpts.CUDAIsDevice) {
       Builder.defineMacro("__HIP_DEVICE_COMPILE__");
+      if (!TI.hasHIPImageSupport())
+        Builder.defineMacro("__HIP_NO_IMAGE_SUPPORT", "1");
+    }
     if (LangOpts.GPUDefaultStream ==
         LangOptions::GPUDefaultStreamKind::PerThread)
       Builder.defineMacro("HIP_API_PER_THREAD_DEFAULT_STREAM");
   }
 }
 
 /// Initialize the predefined C++ language feature test macros defined in
 /// ISO/IEC JTC1/SC22/WG21 (C++) SD-6: "SG10 Feature Test Recommendations".

clang/test/Driver/hip-macros.hip

 // RUN: %clang -E -dM --offload-arch=gfx906 --cuda-device-only -nogpuinc -nogpulib -mcumode \
 // RUN:   -mno-cumode -mno-cumode \
 // RUN:   %s 2>&1 | FileCheck --check-prefixes=CUMODE-ON,WARN-CUMODE %s
 
 // WARN-CUMODE-DAG: warning: ignoring '-mno-cumode' option as it is not currently supported for processor 'gfx906' [-Woption-ignored]
 // WARN-CUMODE-NOT: warning: ignoring '-mno-cumode' option as it is not currently supported for processor 'gfx906' [-Woption-ignored]
 // CUMODE-ON-DAG: #define __AMDGCN_CUMODE__ 1
 // CUMODE-OFF-DAG: #define __AMDGCN_CUMODE__ 0
+
+// RUN: %clang -E -dM --offload-arch=gfx90a --cuda-device-only -nogpuinc -nogpulib \
+// RUN:   %s 2>&1 | FileCheck --check-prefix=IMAGE %s
+// RUN: %clang -E -dM --offload-arch=gfx1100 --cuda-device-only -nogpuinc -nogpulib \
+// RUN:   %s 2>&1 | FileCheck --check-prefix=IMAGE %s
+// RUN: %clang -E -dM --offload-arch=gfx940 --cuda-device-only -nogpuinc -nogpulib \
+// RUN:   %s 2>&1 | FileCheck --check-prefix=NOIMAGE %s
+// RUN: %clang -E -dM --offload-arch=gfx941 --cuda-device-only -nogpuinc -nogpulib \
+// RUN:   %s 2>&1 | FileCheck --check-prefix=NOIMAGE %s
+// RUN: %clang -E -dM --offload-arch=gfx942 --cuda-device-only -nogpuinc -nogpulib \
+// RUN:   %s 2>&1 | FileCheck --check-prefix=NOIMAGE %s
+// IMAGE-NOT: #define __HIP_NO_IMAGE_SUPPORT
+// NOIMAGE: #define __HIP_NO_IMAGE_SUPPORT 1