llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-clang-driver Author: Fabian Ritter (ritter-x2a) <details> <summary>Changes</summary> The `__AMDGCN_WAVEFRONT_SIZE` and `__AMDGCN_WAVEFRONT_SIZE__` macros in HIP can only provide meaningful values during device compilation. They are currently usable in host code, but only contain the default value of 64, independent of the target device(s). This patch redefines them during host compilation to issue a deprecation warning if the macros are used in host code. Their value during host compilation in actual HIP code as well as in preprocessing directives stays 64 as before. Macro uses in preprocessing directives are not diagnosed. Macro uses in device code are not affected. In a later step, after a deprecation period, we can easily adjust this implementation so that macro uses in host code cause hard errors instead of warnings. **Considered Alternatives:** - Introducing a specialized diagnostic during clang's semantic analysis: This is technically possible and allows for cleaner diagnostics, but requires HIP-specific special case handling in clang's very general `Sema::ActOnNumericConstant(...)` method, since these macros appear as integer literals during parsing/semantic analysis where we know if we are in a host function. In comparison, this PR introduces less complexity to code that is independent from HIP. - See also the previous rejected proposal, which eliminates the macros for host compilation: https://github.com/llvm/llvm-project/pull/83558 **Implementation Rationale:** - I have placed the macro redefinitions in a new header file so that it is included even if the `-nogpuinc`, `-nobuiltininc`, and/or `-nostdinc` CLI flags are provided, enabling consistent diagnostics with any combination of these flags. I am open to suggestions for better solutions. - The constexpr function with separate overloads for host and device is a HIP feature that allows us to identify macro uses in host code without special-case handling in the semantic analysis. Their returned value is irrelevant, they are only referenced for the deprecation warning. Constexpr variables cannot be overloaded like this. - The `AMDGCN_WAVEFRONT_SIZE` macros are commonly used in preprocessing directives for conditional includes. The defined expression is carefully crafted to not break this use case: - Calling the constexpr function instead of referencing its value as a function pointer would be diagnosed as an undefined function-like macro by the preprocessor in directives. - Using the more natural comma operator instead of the ternary conditional operator to discard the value of the constexpr function in the expression is illegal in constant expressions that may occur in preprocessing directives according to the Standard (e.g., the C11 Standard, Section 6.6 "Constant expressions", paragraph 3: "Constant expressions shall not contain assignment, increment, decrement, function-call, or comma operators, except when they are contained within a subexpression that is not evaluated.") Clang diagnoses this with -pedantic. - In preprocessing directives, the function identifier is considered an undefined macro, which is interpreted as 0. Implements SWDEV-449015. --- Full diff: https://github.com/llvm/llvm-project/pull/91478.diff 5 Files Affected: - (modified) clang/lib/Driver/ToolChains/AMDGPU.cpp (+11) - (modified) clang/lib/Headers/CMakeLists.txt (+1) - (added) clang/lib/Headers/__clang_hip_device_macro_guards.h (+55) - (added) clang/test/Driver/hip-wavefront-size-host-diagnostics.hip (+52) - (modified) clang/test/Preprocessor/predefined-arch-macros.c (-1) ``````````diff diff --git a/clang/lib/Driver/ToolChains/AMDGPU.cpp b/clang/lib/Driver/ToolChains/AMDGPU.cpp index 07965b487ea79..587aa19349d89 100644 --- a/clang/lib/Driver/ToolChains/AMDGPU.cpp +++ b/clang/lib/Driver/ToolChains/AMDGPU.cpp @@ -550,6 +550,17 @@ void RocmInstallationDetector::AddHIPIncludeArgs(const ArgList &DriverArgs, CC1Args.push_back(DriverArgs.MakeArgString(P)); } + { + // This header implements diagnostics for problematic uses of + // device-specific macros. Since these diagnostics should be issued even + // when GPU headers are not included, this header is included separately. + SmallString<128> P(D.ResourceDir); + llvm::sys::path::append(P, "include"); + CC1Args.push_back("-internal-isystem"); + CC1Args.push_back(DriverArgs.MakeArgString(P)); + CC1Args.append({"-include", "__clang_hip_device_macro_guards.h"}); + } + const auto HandleHipStdPar = [=, &DriverArgs, &CC1Args]() { StringRef Inc = getIncludePath(); auto &FS = D.getVFS(); diff --git a/clang/lib/Headers/CMakeLists.txt b/clang/lib/Headers/CMakeLists.txt index 5f02c71f6ca51..31f1a73fee66a 100644 --- a/clang/lib/Headers/CMakeLists.txt +++ b/clang/lib/Headers/CMakeLists.txt @@ -79,6 +79,7 @@ set(hip_files __clang_hip_math.h __clang_hip_stdlib.h __clang_hip_runtime_wrapper.h + __clang_hip_device_macro_guards.h ) set(hlsl_h diff --git a/clang/lib/Headers/__clang_hip_device_macro_guards.h b/clang/lib/Headers/__clang_hip_device_macro_guards.h new file mode 100644 index 0000000000000..42782c9bb08a7 --- /dev/null +++ b/clang/lib/Headers/__clang_hip_device_macro_guards.h @@ -0,0 +1,55 @@ +/*===---- __clang_hip_device_macro_guards.h - guards for HIP device macros -=== + * + * Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. + * See https://llvm.org/LICENSE.txt for license information. + * SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + * + *===-----------------------------------------------------------------------=== + */ + +/* + * WARNING: This header is intended to be directly -include'd by + * the compiler and is not supposed to be included by users. + * + */ + +#ifndef __CLANG_HIP_DEVICE_MACRO_GUARDS_H__ +#define __CLANG_HIP_DEVICE_MACRO_GUARDS_H__ + +#if __HIP__ +#if !defined(__HIP_DEVICE_COMPILE__) +// The __AMDGCN_WAVEFRONT_SIZE macros cannot hold meaningful values during host +// compilation as devices are not initialized when the macros are defined and +// there may indeed be devices with differing wavefront sizes in the same +// system. This code issues diagnostics when the macros are used in host code. + +#undef __AMDGCN_WAVEFRONT_SIZE +#undef __AMDGCN_WAVEFRONT_SIZE__ + +// Reference __hip_device_macro_guard in a way that is legal in preprocessor +// directives and does not affect the value so that appropriate diagnostics are +// issued. Function calls, casts, or the comma operator would make the macro +// illegal for use in preprocessor directives. +#define __AMDGCN_WAVEFRONT_SIZE (!__hip_device_macro_guard ? 64 : 64) +#define __AMDGCN_WAVEFRONT_SIZE__ (!__hip_device_macro_guard ? 64 : 64) + +// This function is referenced by the macro in device functions during host +// compilation, it SHOULD NOT cause a diagnostic. +__attribute__((device)) static constexpr int __hip_device_macro_guard(void) { + return -1; +} + +// This function is referenced by the macro in host functions during host +// compilation, it SHOULD cause a diagnostic. +__attribute__(( + host, deprecated("The __AMDGCN_WAVEFRONT_SIZE macros do not correspond " + "to the device(s) when used in host code and may only " + "be used in device code."))) static constexpr int +__hip_device_macro_guard(void) { + return -1; +} +// TODO Change "deprecated" to "unavailable" to cause hard errors instead of +// warnings. +#endif +#endif // __HIP__ +#endif // __CLANG_HIP_DEVICE_MACRO_GUARDS_H__ diff --git a/clang/test/Driver/hip-wavefront-size-host-diagnostics.hip b/clang/test/Driver/hip-wavefront-size-host-diagnostics.hip new file mode 100644 index 0000000000000..e0ee44cdc2986 --- /dev/null +++ b/clang/test/Driver/hip-wavefront-size-host-diagnostics.hip @@ -0,0 +1,52 @@ +// REQUIRES: amdgpu-registered-target +// RUN: %clang -xhip --offload-arch=gfx1030 --offload-host-only -pedantic -nogpuinc -nogpulib -nobuiltininc -nostdinc -fsyntax-only -Xclang -verify=onhost %s +// RUN: %clang -xhip --offload-arch=gfx1030 --offload-device-only -pedantic -nogpuinc -nogpulib -nobuiltininc -nostdinc -fsyntax-only -Xclang -verify=ondevice %s + +// ondevice-no-diagnostics + +#define WRAPPED __AMDGCN_WAVEFRONT_SIZE__ + +__attribute__((host, device)) void use(int, const char*); + +template<int N> __attribute__((host, device)) int templatify(int x) { + return x + N; +} + +// no warning expected +#if defined(__HIP_DEVICE_COMPILE__) && (__AMDGCN_WAVEFRONT_SIZE__ == 64) && (__AMDGCN_WAVEFRONT_SIZE == 64) +int foo(void); +#endif + +// no warning expected +__attribute__((device)) int device_var = __AMDGCN_WAVEFRONT_SIZE__; + +__attribute__((device)) +void device_fun() { + // no warnings expected + use(__AMDGCN_WAVEFRONT_SIZE, "device function"); + use(__AMDGCN_WAVEFRONT_SIZE__, "device function"); + use(WRAPPED, "device function"); + use(templatify<__AMDGCN_WAVEFRONT_SIZE__>(42), "device function"); +} + +// warning expected +int host_var = __AMDGCN_WAVEFRONT_SIZE__; // onhost-warning {{'__hip_device_macro_guard' is deprecated: The __AMDGCN_WAVEFRONT_SIZE macros do not correspond to the device(s) when used in host code and may only be used in device code.}} + +__attribute__((host)) +void host_fun() { + // warnings expected + use(__AMDGCN_WAVEFRONT_SIZE, "host function"); // onhost-warning {{'__hip_device_macro_guard' is deprecated: The __AMDGCN_WAVEFRONT_SIZE macros do not correspond to the device(s) when used in host code and may only be used in device code.}} + use(__AMDGCN_WAVEFRONT_SIZE__, "host function"); // onhost-warning {{'__hip_device_macro_guard' is deprecated: The __AMDGCN_WAVEFRONT_SIZE macros do not correspond to the device(s) when used in host code and may only be used in device code.}} + use(WRAPPED, "host function"); // onhost-warning {{'__hip_device_macro_guard' is deprecated: The __AMDGCN_WAVEFRONT_SIZE macros do not correspond to the device(s) when used in host code and may only be used in device code.}} + use(templatify<__AMDGCN_WAVEFRONT_SIZE__>(42), "host function"); // onhost-warning {{'__hip_device_macro_guard' is deprecated: The __AMDGCN_WAVEFRONT_SIZE macros do not correspond to the device(s) when used in host code and may only be used in device code.}} +} + +__attribute((host, device)) +void host_device_fun() { + // warnings expected + use(__AMDGCN_WAVEFRONT_SIZE__, "host device function"); // onhost-warning {{'__hip_device_macro_guard' is deprecated: The __AMDGCN_WAVEFRONT_SIZE macros do not correspond to the device(s) when used in host code and may only be used in device code.}} + use(WRAPPED, "host device function"); // onhost-warning {{'__hip_device_macro_guard' is deprecated: The __AMDGCN_WAVEFRONT_SIZE macros do not correspond to the device(s) when used in host code and may only be used in device code.}} + use(templatify<__AMDGCN_WAVEFRONT_SIZE__>(42), "host device function"); // onhost-warning {{'__hip_device_macro_guard' is deprecated: The __AMDGCN_WAVEFRONT_SIZE macros do not correspond to the device(s) when used in host code and may only be used in device code.}} +} + +// onhost-note@__clang_hip_device_macro_guards.h:45 0+ {{'__hip_device_macro_guard' has been explicitly marked deprecated here}} diff --git a/clang/test/Preprocessor/predefined-arch-macros.c b/clang/test/Preprocessor/predefined-arch-macros.c index ca51f2fc22c51..ee3e26f203964 100644 --- a/clang/test/Preprocessor/predefined-arch-macros.c +++ b/clang/test/Preprocessor/predefined-arch-macros.c @@ -4340,7 +4340,6 @@ // RUN: %clang -x hip -E -dM %s -o - 2>&1 --offload-host-only -nogpulib \ // RUN: -nogpuinc --offload-arch=gfx803 -target x86_64-unknown-linux \ // RUN: | FileCheck -match-full-lines %s -check-prefixes=CHECK_HIP_HOST -// CHECK_HIP_HOST: #define __AMDGCN_WAVEFRONT_SIZE__ 64 // CHECK_HIP_HOST: #define __AMDGPU__ 1 // CHECK_HIP_HOST: #define __AMD__ 1 `````````` </details> https://github.com/llvm/llvm-project/pull/91478 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
