[PATCH] D145343: [AMDGPU] Emit predefined macro `__AMDGCN_CUMODE_OPTION`

[Matt Arsenault via Phabricator via cfe-commits]

arsenm requested changes to this revision.
arsenm added a comment.
This revision now requires changes to proceed.

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


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

https://reviews.llvm.org/D145343

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145343: [AMDGPU] Emit predefined macro `__AMDGCN_CUMODE_OPTION`

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

Can `__has_feature` be used to accomplish what you need?


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

https://reviews.llvm.org/D145343

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145343: [AMDGPU] Emit predefined macro `__AMDGCN_CUMODE_OPTION`

[Brian Sumner via Phabricator via cfe-commits]

b-sumner added a comment.

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?

I think applications may need to check if CUMode is enabled at compile time and 
select code based on that.  But a concern has been raised about compiling such 
source with an older compiler which is not setting the macro regardless of 
whether -mcumode was used.   The conservative approach here would be to only 
define a macro only if -mcumode is used, and define nothing if it is not used.  
Then, when using an old compiler, the code will assume -mno-cumode which is 
always fine to do.


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

https://reviews.llvm.org/D145343

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145343: [AMDGPU] Emit predefined macro `__AMDGCN_CUMODE_OPTION`

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

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?


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

https://reviews.llvm.org/D145343

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145343: [AMDGPU] Emit predefined macro `__AMDGCN_CUMODE_OPTION`

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

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


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

https://reviews.llvm.org/D145343

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145343: [AMDGPU] Emit predefined macro `__AMDGCN_CUMODE_OPTION`

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl created this revision.
yaxunl added reviewers: tra, b-sumner, arsenm.
Herald added subscribers: kosarev, kerbowa, tpr, dstuttard, jvesely, kzhuravl.
Herald added a project: All.
yaxunl requested review of this revision.
Herald added a subscriber: wdng.

Predefine `__AMDGCN_CUMODE_OPTION` as 1 or 0 when -mcumode or -mno-cumode
is specified.

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)


https://reviews.llvm.org/D145343

Files:
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/AMDGPU.h
  clang/test/Driver/amdgpu-macros.cl
  clang/test/Driver/hip-macros.hip


Index: clang/test/Driver/hip-macros.hip
===
--- clang/test/Driver/hip-macros.hip
+++ clang/test/Driver/hip-macros.hip
@@ -19,3 +19,14 @@
 // 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
Index: clang/test/Driver/amdgpu-macros.cl
===
--- clang/test/Driver/amdgpu-macros.cl
+++ clang/test/Driver/amdgpu-macros.cl
@@ -156,3 +156,14 @@
 // 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
Index: clang/lib/Basic/Targets/AMDGPU.h
===
--- clang/lib/Basic/Targets/AMDGPU.h
+++ clang/lib/Basic/Targets/AMDGPU.h
@@ -43,6 +43,9 @@
   unsigned GPUFeatures;
   unsigned WavefrontSize;
 
+  /// User explicitly specify -mcumode or -mno-cumode.
+  std::optional 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.
@@ -443,6 +446,10 @@
   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))
Index: clang/lib/Basic/Targets/AMDGPU.cpp
===
--- clang/lib/Basic/Targets/AMDGPU.cpp
+++ clang/lib/Basic/Targets/AMDGPU.cpp
@@ -487,6 +487,8 @@
 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) {


Index: clang/test/Driver/hip-macros.hip
===
--- clang/test/Driver/hip-macros.hip
+++ clang/test/Driver/hip-macros.hip
@@ -19,3 +19,14 @@
 // 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