[AMDGPU] Emit predefined macro `__AMDGCN_CUMODE__`

[yaxunl]

Summary

Predefine __AMDGCN_CUMODE__ as 1 or 0 when compilation assumes CU or WGP modes.

If WGP mode is not supported, ignore -mno-cumode and emit a warning.

This is needed for implementing device functions like __smid (https://github.com/ROCm-Developer-Tools/hipamd/blob/312dff7b794337aa040be0691acc78e9f968a8d2/include/hip/amd_detail/amd_device_functions.h#L957)

[arsenm]

I think exposing whether or not the flag was used is weird/broken, as is including _OPTION in the name. Should just define to whether it's enabled or not

[yaxunl]

In D145343#4170250, @arsenm wrote:

I think exposing whether or not the flag was used is weird/broken, as is including _OPTION in the name. Should just define to whether it's enabled or not

I agree. @b-sumner What do you think?

[b-sumner]

In D145343#4170305, @yaxunl wrote:

In D145343#4170250, @arsenm wrote:

I think exposing whether or not the flag was used is weird/broken, as is including _OPTION in the name. Should just define to whether it's enabled or not

I agree. @b-sumner What do you think?

Applications may need to check if CUMode is set at compile time, and they may also need to check if an old compiler is being used. So an macro that says whether it's enabled or not is fine, and if no macro is defined, then the application can conclude the compiler is old

[tra]

Can __has_feature be used to accomplish what you need?

[arsenm]

We can’t let specific flag usage leak into the semantics. Cu mode is on or off. If someone really cares about supporting older compilers they could always define their own macro

[yaxunl]

Revised by Matt's comments.

[yaxunl]

In D145343#4172643, @tra wrote:

Can __has_feature be used to accomplish what you need?

There are a few issues using __has_feature or similar builtin macros to check target feature, which has been discussed in https://reviews.llvm.org/D125555 . One of the concerns is the stability of target feature name.

Using a predefined macro does not have such concern.

[arsenm]

clang/lib/Basic/Targets/AMDGPU.cpp

490

Why do we sometimes use __ on both sides, and sometimes only leading __?

[yaxunl]

clang/lib/Basic/Targets/AMDGPU.cpp

490

We did not establish a naming convention for pre-defined macros.

The rationale of only prefixing with __ is that:

1. it is shorter

2. many other targets following that for target feature related macros, e.g. https://github.com/llvm/llvm-project/blob/main/clang/test/Preprocessor/arm-target-features.c , https://github.com/llvm/llvm-project/blob/main/clang/test/Preprocessor/riscv-target-features.c

The rationale of adding __ on both sides is:

1. most gcc predefined macros follow this convention https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html

2. some targets follow this convention for target feature related macros, e.g https://github.com/llvm/llvm-project/blob/main/clang/test/Preprocessor/x86_target_features.c

I think it is not too late to establish a naming convention for predefined macros for amdgpu target. I would prefer to adding __ on both sides since

1. most of the existing amdgpu predefined macros follow that. We just need to add __AMDGCN_WAVEFRONT_SIZE__ and phasing out __AMDGCN_WAVEFRONT_SIZE. This will be the least interruptive.

2. we will conform to GCC naming convention for pre-defined macros.

Or maybe we should have an RFC for this topic.

[yaxunl]

rename the macro to be consistent with most existing predefined macros for amdgcn target

[arsenm]

clang/include/clang/Basic/DiagnosticDriverKinds.td

121–123 ↗

(On Diff #521396)

I'm surprised there already isn't a warning for this?

clang/lib/Driver/ToolChains/CommonArgs.cpp

146 ↗

(On Diff #521396)

I don't understand the use of "no-cumode". Where is this defined?

[yaxunl]

clang/include/clang/Basic/DiagnosticDriverKinds.td

121–123 ↗

(On Diff #521396)

I am surprised too. Before this patch, there is no check and no handling of unsupported target features in Clang. clang just blindly adds target features specified in the command line and relies on the backend to handle them properly.

This patch enables Clang to warn and ignore unsupported target features.

clang/lib/Driver/ToolChains/CommonArgs.cpp

146 ↗

(On Diff #521396)

This function is called by clang driver. The argument TargetFeature is the target feature command line option to be checked, with -m removed, e.g. -mcumode or -mno-cumode passed to this function as "cumode" or "no-cumode".

[arsenm]

clang/lib/Driver/ToolChains/CommonArgs.cpp

146 ↗

(On Diff #521396)

Can this use something like

if (!DriverArgs.hasFlag(options::OPT_fgpu_sanitize,
                        options::OPT_fno_gpu_sanitize, true))
  return true;

to make it clear this is checking the flag? At a minimum this needs to be renamed to indicate it's a flag and not the actual subtarget feature name

[yaxunl]

revised by Matt's comments

clang/lib/Basic/Targets/AMDGPU.h

class LLVM_LIBRARY_VISIBILITY AMDGPUTargetInfo final : public TargetInfo {
   };
   static const LangASMap AMDGPUDefIsGenMap;
   static const LangASMap AMDGPUDefIsPrivMap;
 
   llvm::AMDGPU::GPUKind GPUKind;
   unsigned GPUFeatures;
   unsigned WavefrontSize;
 
+  /// User explicitly specify -mcumode or -mno-cumode.
+  std::optional<bool> CUModeOpt;
+
   /// 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;
 
public:
   bool handleTargetFeatures(std::vector<std::string> &Features,
                             DiagnosticsEngine &Diags) override {
     auto TargetIDFeatures =
         getAllPossibleTargetIDFeatures(getTriple(), getArchNameAMDGCN(GPUKind));
     for (const auto &F : Features) {
       assert(F.front() == '+' || F.front() == '-');
       if (F == "+wavefrontsize64")
         WavefrontSize = 64;
+      else if (F == "+cumode")
+        CUModeOpt = true;
+      else if (F == "-cumode")
+        CUModeOpt = false;
       bool IsOn = F.front() == '+';
       StringRef Name = StringRef(F).drop_front();
       if (!llvm::is_contained(TargetIDFeatures, Name))
         continue;
       assert(OffloadArchFeatures.find(Name) == OffloadArchFeatures.end());
       OffloadArchFeatures[Name] = IsOn;
     }
     return true;

clang/lib/Basic/Targets/AMDGPU.cpp

void AMDGPUTargetInfo::getTargetDefines(const LangOptions &Opts,
   if (hasLDEXPF())
     Builder.defineMacro("__HAS_LDEXPF__");
   if (hasFP64())
     Builder.defineMacro("__HAS_FP64__");
   if (hasFastFMA())
     Builder.defineMacro("FP_FAST_FMA");
 
   Builder.defineMacro("__AMDGCN_WAVEFRONT_SIZE", Twine(WavefrontSize));
+  if (CUModeOpt)
+    Builder.defineMacro("__AMDGCN_CUMODE_OPTION", Twine(CUModeOpt.value()));
 }
 
 void AMDGPUTargetInfo::setAuxTarget(const TargetInfo *Aux) {
   assert(HalfFormat == Aux->HalfFormat);
   assert(FloatFormat == Aux->FloatFormat);
   assert(DoubleFormat == Aux->DoubleFormat);
 
   // On x86_64 long double is 80-bit extended precision format, which is

clang/test/Driver/amdgpu-macros.cl

 // RUN: %clang -E -dM -target amdgcn -mcpu=gfx1010 -mwavefrontsize64 \
 // RUN:   -mno-wavefrontsize64 %s 2>&1 | FileCheck --check-prefix=WAVE32 %s
 // RUN: %clang -E -dM -target amdgcn -mcpu=gfx906 -mno-wavefrontsize64 \
 // RUN:   -mwavefrontsize64 %s 2>&1 | FileCheck --check-prefix=WAVE64 %s
 // RUN: %clang -E -dM -target amdgcn -mcpu=gfx1010 -mno-wavefrontsize64 \
 // RUN:   -mwavefrontsize64 %s 2>&1 | FileCheck --check-prefix=WAVE64 %s
 // WAVE64-DAG: #define __AMDGCN_WAVEFRONT_SIZE 64
 // WAVE32-DAG: #define __AMDGCN_WAVEFRONT_SIZE 32
+
+// RUN: %clang -E -dM -target amdgcn -mcpu=gfx1030 \
+// RUN:   %s 2>&1 | FileCheck --check-prefix=CUMODE-DEFAULT %s
+// RUN: %clang -E -dM -target amdgcn -mcpu=gfx1030 -mcumode \
+// RUN:   %s 2>&1 | FileCheck --check-prefix=CUMODE-ON %s
+// RUN: %clang -E -dM -target amdgcn -mcpu=gfx1030 -mno-cumode \
+// RUN:   %s 2>&1 | FileCheck --check-prefix=CUMODE-OFF %s
+// CUMODE-DEFAULT-NOT: #define __AMDGCN_CUMODE_OPTION
+// CUMODE-DEFAULT-NOT: #define __AMDGCN_CUMODE_OPTION
+// CUMODE-ON-DAG: #define __AMDGCN_CUMODE_OPTION 1
+// CUMODE-OFF-DAG: #define __AMDGCN_CUMODE_OPTION 0

clang/test/Driver/hip-macros.hip

 // RUN: %clang -E -dM --offload-arch=gfx906 -mno-wavefrontsize64 \
 // RUN:   --cuda-device-only -nogpuinc -nogpulib \
 // RUN:   -mwavefrontsize64 %s 2>&1 | FileCheck --check-prefixes=WAVE64 %s
 // RUN: %clang -E -dM --offload-arch=gfx1010 -mno-wavefrontsize64 \
 // RUN:   --cuda-device-only -nogpuinc -nogpulib \
 // RUN:   -mwavefrontsize64 %s 2>&1 | FileCheck --check-prefixes=WAVE64 %s
 // WAVE64-DAG: #define __AMDGCN_WAVEFRONT_SIZE 64
 // WAVE32-DAG: #define __AMDGCN_WAVEFRONT_SIZE 32
+
+// RUN: %clang -E -dM --offload-arch=gfx1030 --cuda-device-only -nogpuinc -nogpulib \
+// RUN:   %s 2>&1 | FileCheck --check-prefix=CUMODE-DEFAULT %s
+// RUN: %clang -E -dM --offload-arch=gfx1030 --cuda-device-only -nogpuinc -nogpulib -mcumode \
+// RUN:   %s 2>&1 | FileCheck --check-prefix=CUMODE-ON %s
+// RUN: %clang -E -dM --offload-arch=gfx1030 --cuda-device-only -nogpuinc -nogpulib -mno-cumode \
+// RUN:   %s 2>&1 | FileCheck --check-prefix=CUMODE-OFF %s
+// CUMODE-DEFAULT-NOT: #define __AMDGCN_CUMODE_OPTION
+// CUMODE-DEFAULT-NOT: #define __AMDGCN_CUMODE_OPTION
+// CUMODE-ON-DAG: #define __AMDGCN_CUMODE_OPTION 1
+// CUMODE-OFF-DAG: #define __AMDGCN_CUMODE_OPTION 0