[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

457

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/DiagnosticFrontendKinds.td

def warn_fe_backend_unsupported_fp_rounding : Warning<
     "overriding currently unsupported rounding mode on this target">,
     InGroup<UnsupportedFPOpt>;
 def warn_fe_backend_unsupported_fp_exceptions : Warning<
     "overriding currently unsupported use of floating point exceptions "
     "on this target">, InGroup<UnsupportedFPOpt>;
 def warn_fe_backend_invalid_feature_flag : Warning<
     "feature flag '%0' must start with either '+' to enable the feature or '-'"
     " to disable it; flag ignored">, InGroup<InvalidCommandLineArgument>;
+def warn_fe_backend_readonly_feature_flag : Warning<
+    "feature flag '%0' is ignored since the feature is read only">,
+    InGroup<InvalidCommandLineArgument>;
 
 def err_incompatible_fp_eval_method_options : Error<
     "option 'ffp-eval-method' cannot be used with option "
     "%select{'fapprox-func'|'mreassociate'|'freciprocal'}0">;
 
 def remark_fe_backend_optimization_remark : Remark<"%0">, BackendInfo,
     InGroup<BackendOptimizationRemark>;
 def remark_fe_backend_optimization_remark_missed : Remark<"%0">, BackendInfo,

clang/include/clang/Basic/TargetInfo.h

 #include "clang/Basic/TargetOptions.h"
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Frontend/OpenMP/OMPGridValues.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/Support/DataTypes.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/VersionTuple.h"
 #include "llvm/TargetParser/Triple.h"
 #include <cassert>
 #include <optional>
protected:
 
   // TargetInfo Constructor.  Default initializes all fields.
   TargetInfo(const llvm::Triple &T);
 
   // UserLabelPrefix must match DL's getGlobalPrefix() when interpreted
   // as a DataLayout object.
   void resetDataLayout(StringRef DL, const char *UserLabelPrefix = "");
 
+  // Target features that are read-only and should not be disabled/enabled
+  // by command line options. Such features are for emitting predefined
+  // macros or checking availability of builtin functions and can be omitted
+  // in function attributes in IR.
+  llvm::StringSet<> ReadOnlyFeatures;
+
 public:
   /// Construct a target for the given options.
   ///
   /// \param Opts - The options to use to initialize the target. The target may
   /// modify the options to canonicalize the target feature information to match
   /// what the backend expects.
   static TargetInfo *
   CreateTargetInfo(DiagnosticsEngine &Diags,
virtual bool handleTargetFeatures(std::vector<std::string> &Features,
     return true;
   }
 
   /// Determine whether the given target has the given feature.
   virtual bool hasFeature(StringRef Feature) const {
     return false;
   }
 
+  /// Determine whether the given target feature is read only.
+  bool isReadOnlyFeature(StringRef Feature) const {
+    return ReadOnlyFeatures.count(Feature);
+  }
+
   /// Identify whether this target supports multiversioning of functions,
   /// which requires support for cpu_supports and cpu_is functionality.
   bool supportsMultiVersioning() const {
     return getTriple().isX86() || getTriple().isAArch64();
   }
 
   /// Identify whether this target supports IFuncs.
   bool supportsIFunc() const {
#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/TargetInfo.cpp

bool TargetInfo::initFeatureMap(
     const std::vector<std::string> &FeatureVec) const {
   for (const auto &F : FeatureVec) {
     StringRef Name = F;
     if (Name.empty())
       continue;
     // Apply the feature via the target.
     if (Name[0] != '+' && Name[0] != '-')
       Diags.Report(diag::warn_fe_backend_invalid_feature_flag) << Name;
+    else if (isReadOnlyFeature(Name.substr(1)))
+      Diags.Report(diag::warn_fe_backend_readonly_feature_flag) << Name;
     else
       setFeatureEnabled(Features, Name.substr(1), Name[0] == '+');
   }
   return true;
 }
 
 ParsedTargetAttr TargetInfo::parseTargetAttr(StringRef Features) const {
   ParsedTargetAttr Ret;

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 = false;
+
   /// 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;
 
bool handleTargetFeatures(std::vector<std::string> &Features,
     for (const auto &F : Features) {
       assert(F.front() == '+' || F.front() == '-');
       if (F == "+wavefrontsize64")
         WavefrontSize = 64;
       else if (F == "+cumode")
         CUMode = true;
       else if (F == "-cumode")
         CUMode = false;
+      else if (F == "+image-insts")
+        HasImage = true;
       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);
+  ReadOnlyFeatures.insert("image-insts");
 }
 
 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 ||
                      !isAMDGCN(getTriple()));

clang/lib/CodeGen/CodeGenModule.cpp

if (!TargetCPU.empty()) {
     Attrs.addAttribute("target-cpu", TargetCPU);
     AddedAttr = true;
   }
   if (!TuneCPU.empty()) {
     Attrs.addAttribute("tune-cpu", TuneCPU);
     AddedAttr = true;
   }
   if (!Features.empty() && SetTargetFeatures) {
+    llvm::erase_if(Features, [&](const std::string& F) {
+       return getTarget().isReadOnlyFeature(F.substr(1));
+    });
     llvm::sort(Features);
     Attrs.addAttribute("target-features", llvm::join(Features, ","));
     AddedAttr = true;
   }
 
   return AddedAttr;
 }
 

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-prefixes=IMAGE,NOWARN %s
+// RUN: %clang -E -dM --offload-arch=gfx1100 --cuda-device-only -nogpuinc -nogpulib \
+// RUN:   %s 2>&1 | FileCheck --check-prefixes=IMAGE,NOWARN %s
+// RUN: %clang -E -dM --offload-arch=gfx940 --cuda-device-only -nogpuinc -nogpulib \
+// RUN:   %s 2>&1 | FileCheck --check-prefixes=NOIMAGE,NOWARN %s
+// RUN: %clang -E -dM --offload-arch=gfx941 --cuda-device-only -nogpuinc -nogpulib \
+// RUN:   %s 2>&1 | FileCheck --check-prefixes=NOIMAGE,NOWARN %s
+// RUN: %clang -E -dM --offload-arch=gfx942 --cuda-device-only -nogpuinc -nogpulib \
+// RUN:   %s 2>&1 | FileCheck --check-prefixes=NOIMAGE,NOWARN %s
+// RUN: %clang -E -dM --offload-arch=gfx1100 --cuda-device-only -nogpuinc -nogpulib \
+// RUN:   -Xclang -target-feature -Xclang "-image-insts" %s 2>&1 | FileCheck --check-prefixes=IMAGE,WARN %s
+// RUN: %clang -E -dM --offload-arch=gfx940 --cuda-device-only -nogpuinc -nogpulib \
+// RUN:   -Xclang -target-feature -Xclang "+image-insts" %s 2>&1 | FileCheck --check-prefixes=NOIMAGE,WARN %s
+// NOWARN-NOT: warning
+// WARN: warning: feature flag '{{[+|-]}}image-insts' is ignored since the feature is read only [-Winvalid-command-line-argument]
+// IMAGE-NOT: #define __HIP_NO_IMAGE_SUPPORT
+// NOIMAGE: #define __HIP_NO_IMAGE_SUPPORT 1

llvm/lib/TargetParser/TargetParser.cpp

case GK_GFX1100:
       Features["16-bit-insts"] = true;
       Features["dpp"] = true;
       Features["gfx8-insts"] = true;
       Features["gfx9-insts"] = true;
       Features["gfx10-insts"] = true;
       Features["gfx10-3-insts"] = true;
       Features["gfx11-insts"] = true;
       Features["atomic-fadd-rtn-insts"] = true;
+      Features["image-insts"] = true;
       break;
     case GK_GFX1036:
     case GK_GFX1035:
     case GK_GFX1034:
     case GK_GFX1033:
     case GK_GFX1032:
     case GK_GFX1031:
     case GK_GFX1030:
       Features["ci-insts"] = true;
       Features["dot1-insts"] = true;
       Features["dot2-insts"] = true;
       Features["dot5-insts"] = true;
       Features["dot6-insts"] = true;
       Features["dot7-insts"] = true;
       Features["dot10-insts"] = true;
       Features["dl-insts"] = true;
       Features["16-bit-insts"] = true;
       Features["dpp"] = true;
       Features["gfx8-insts"] = true;
       Features["gfx9-insts"] = true;
       Features["gfx10-insts"] = true;
       Features["gfx10-3-insts"] = true;
+      Features["image-insts"] = true;
       Features["s-memrealtime"] = true;
       Features["s-memtime-inst"] = true;
       break;
     case GK_GFX1012:
     case GK_GFX1011:
       Features["dot1-insts"] = true;
       Features["dot2-insts"] = true;
       Features["dot5-insts"] = true;
       Features["dot6-insts"] = true;
       Features["dot7-insts"] = true;
       Features["dot10-insts"] = true;
       [[fallthrough]];
     case GK_GFX1013:
     case GK_GFX1010:
       Features["dl-insts"] = true;
       Features["ci-insts"] = true;
       Features["16-bit-insts"] = true;
       Features["dpp"] = true;
       Features["gfx8-insts"] = true;
       Features["gfx9-insts"] = true;
       Features["gfx10-insts"] = true;
+      Features["image-insts"] = true;
       Features["s-memrealtime"] = true;
       Features["s-memtime-inst"] = true;
       break;
     case GK_GFX942:
     case GK_GFX941:
     case GK_GFX940:
       Features["gfx940-insts"] = true;
       Features["fp8-insts"] = true;
       Features["atomic-ds-pk-add-16-insts"] = true;
       Features["atomic-flat-pk-add-16-insts"] = true;
       Features["atomic-global-pk-add-bf16-inst"] = true;
-      [[fallthrough]];
+      Features["gfx90a-insts"] = true;
+      Features["atomic-buffer-global-pk-add-f16-insts"] = true;
+      Features["atomic-fadd-rtn-insts"] = true;
+      Features["dot3-insts"] = true;
+      Features["dot4-insts"] = true;
+      Features["dot5-insts"] = true;
+      Features["dot6-insts"] = true;
+      Features["mai-insts"] = true;
+      Features["dl-insts"] = true;
+      Features["dot1-insts"] = true;
+      Features["dot2-insts"] = true;
+      Features["dot7-insts"] = true;
+      Features["dot10-insts"] = true;
+      Features["gfx9-insts"] = true;
+      Features["gfx8-insts"] = true;
+      Features["16-bit-insts"] = true;
+      Features["dpp"] = true;
+      Features["s-memrealtime"] = true;
+      Features["ci-insts"] = true;
+      Features["s-memtime-inst"] = true;
+      break;
     case GK_GFX90A:
       Features["gfx90a-insts"] = true;
       Features["atomic-buffer-global-pk-add-f16-insts"] = true;
       Features["atomic-fadd-rtn-insts"] = true;
       [[fallthrough]];
     case GK_GFX908:
       Features["dot3-insts"] = true;
       Features["dot4-insts"] = true;
if (T.isAMDGCN()) {
     case GK_GFX702:
     case GK_GFX701:
     case GK_GFX700:
       Features["ci-insts"] = true;
       [[fallthrough]];
     case GK_GFX602:
     case GK_GFX601:
     case GK_GFX600:
+      Features["image-insts"] = true;
       Features["s-memtime-inst"] = true;
       break;
     case GK_NONE:
       break;
     default:
       llvm_unreachable("Unhandled GPU!");
     }
   } else {