[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

pdhaliwal added inline comments. Comment at: clang/lib/Headers/openmp_wrappers/__clang_openmp_device_functions.h:38 + +#pragma omp begin declare variant match(device = {arch(amdgcn)}) + JonChesterfield wrote: > Given that declare variant didn't work elsewhere, i

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added inline comments. Comment at: clang/lib/Headers/openmp_wrappers/__clang_openmp_device_functions.h:38 + +#pragma omp begin declare variant match(device = {arch(amdgcn)}) + Given that declare variant didn't work elsewhere, it probably doesn't

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG713a5d12cde5: [OpenMP][AMDGCN] Initial math headers support (authored by pdhaliwal). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTIO

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield accepted this revision. JonChesterfield added a comment. Latest patch can only misfire on amdgpu so lets go with it and try to work out variant vs ifdef subsequently. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https:

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added inline comments. Comment at: clang/lib/Headers/openmp_wrappers/math.h:53 +#pragma omp begin declare variant match(device = {arch(amdgcn)}) + +#define __OPENMP_AMDGCN__ That's quite worrying. Declare variant match amdgcn is supposed to have t

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

pdhaliwal added a comment. @ye-luo and @JonChesterfield can you please test the latest version of this patch? It should work now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904 _

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

pdhaliwal updated this revision to Diff 363387. pdhaliwal added a comment. Fixed compilation error for nvptx headers. Tested on both cuda and non-cuda systems. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added a comment. In D104904#2916943 , @ye-luo wrote: > Unforuantely I hit error on my ubuntu 20.04 system. > > #include > int main() > { } Given that ^ in fail.cpp and an invocation on a machine that doesn't have cuda or an nvidia

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added a comment. This patch didn't change complex so I'm struggling to make sense of the backtrace. Something in libstdc++ needs memory but doesn't include it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://revie

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

ye-luo added a comment. Unforuantely I hit error #include int main() { } ~/opt/llvm-clang/build_mirror_offload_main/bin/clang++ -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa -Xopenmp-target=amdgcn-amd-amdhsa -march=gfx906 main.cpp -c works fine ~/opt/llvm-clang/build_mirror_offload_main/

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG12da97ea10a9: [OpenMP][AMDGCN] Initial math headers support (authored by pdhaliwal). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTIO

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

pdhaliwal updated this revision to Diff 363090. pdhaliwal added a comment. Addressed review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904 Files: clang/lib/Driver/ToolChains/Clang.cpp clang

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield accepted this revision. JonChesterfield added inline comments. Comment at: clang/lib/Headers/openmp_wrappers/cmath:113 +__DEVICE__ float lgamma(float __x) { return ::lgammaf(__x); } +//__DEVICE__ long long int llrint(float __x) { return ::llrintf(__x); } +//__DEVI

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

pdhaliwal updated this revision to Diff 362997. pdhaliwal added a comment. It required some work to fix the failing lit test case. And many thanks to @estewart for helping in that. The current status is that we are now following the nvptx openmp strategy for openmp math headers very closely. In t

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield reopened this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Landing ocml side first seems reasonable as it's less likely to be broken and makes testing this more straightforward Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

estewart08 added a comment. In D104904#2913983 , @ye-luo wrote: > how to get this moving? We are working on some additions to this patch. The lit failure noted above has been fixed locally. I would expect an update here very soon. Repository: rG LLV

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

ye-luo added a comment. how to get this moving? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added a comment. cstdlib test header contains // amdgcn already provides definition of fabs #ifndef __AMDGCN__ float fabs(float __x) { return __builtin_fabs(__x); } #endif If I delete or invert the ifndef > $HOME/llvm-build/llvm/lib/clang/13.0.0/include/__clang_hip_cmath

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added a comment. Thanks! Will take a look. Feel free to revert, I'll do so shortly if noone beats me to it Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

thakis added a comment. This breaks tests: http://45.33.8.238/linux/51733/step_7.txt Please take a look and revert for now if it takes a while to fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

hliao added a comment. My local build failed due to regression failures. clang/test/Headers/openmp_device_math_isnan.cpp failed with the following errors on undeclared `fabs`. 1950 /home/michliao/working/llvm/llvm-project/clang/test/Headers/Inputs/include/cstdlib:29:31: error: use of undeclar

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added a comment. Landed on @pdhaliwal's behalf. My expectation is that this patch mostly works and the rough edges can be cleaned up once ocml is linked in and we can more easily run more applications through it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG968899ad9cf1: [OpenMP][AMDGCN] Initial math headers support (authored by pdhaliwal, committed by JonChesterfield). Repository: rG LLVM Github Mono

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield updated this revision to Diff 360447. JonChesterfield added a comment. - rebase on main Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904 Files: clang/lib/Driver/ToolChains/Clang.cpp cla

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

estewart08 added inline comments. Comment at: clang/test/Headers/Inputs/include/cstdlib:29 float fabs(float __x) { return __builtin_fabs(__x); } +#endif jdoerfert wrote: > JonChesterfield wrote: > > jdoerfert wrote: > > > estewart08 wrote: > > > > JonChesterfi

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. D105221 so LGTM too Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://revie

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

jdoerfert accepted this revision. jdoerfert added a comment. LG from my side Comment at: clang/test/Headers/Inputs/include/cstdlib:29 float fabs(float __x) { return __builtin_fabs(__x); } +#endif JonChesterfield wrote: > jdoerfert wrote: > > estewart08 wrote

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added a comment. Only revision I'm looking for here is to land D105221 or equivalent first Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904 _

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added inline comments. Comment at: clang/test/Headers/Inputs/include/cstdlib:29 float fabs(float __x) { return __builtin_fabs(__x); } +#endif jdoerfert wrote: > estewart08 wrote: > > JonChesterfield wrote: > > > jdoerfert wrote: > > > > That se

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

jdoerfert added inline comments. Comment at: clang/test/Headers/Inputs/include/cstdlib:29 float fabs(float __x) { return __builtin_fabs(__x); } +#endif estewart08 wrote: > JonChesterfield wrote: > > jdoerfert wrote: > > > That seems to be fundamentally broken

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

estewart08 added inline comments. Comment at: clang/test/Headers/Inputs/include/cstdlib:29 float fabs(float __x) { return __builtin_fabs(__x); } +#endif JonChesterfield wrote: > jdoerfert wrote: > > That seems to be fundamentally broken then, but let's see, ma

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added inline comments. Comment at: clang/lib/Headers/openmp_wrappers/math.h:41 #pragma omp begin declare variant match( \ device = {arch(nvptx, nvptx64)}, implementation = {extension(match_any)}) tian

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

tianshilei1992 added inline comments. Comment at: clang/lib/Headers/openmp_wrappers/math.h:41 #pragma omp begin declare variant match( \ device = {arch(nvptx, nvptx64)}, implementation = {extension(match_any)}) JonCh

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added a comment. tagged request changes because I think we should ifdef around complex before (or while) landing this, as defining `__CUDA__`, even transiently, is a user hostile thing to do from amdgpu openmp It is *really* ugly that we have cuda and hip implementations of cmat

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added a comment. Weird pre-existing stuff in cuda_complex_builtins. It has an #ifdef AMDGCN macro in it, despite 'cuda' in the name. I note there is no corresponding 'hip' complex builtins. The ifdef logic for stubbing out some functions (which is done with macros...) isn't ide

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added a comment. Good spot. I've been feeding the following to various toolchains: // permute //#include //#include //#include #ifndef _OPENMP #error "OpenMP should be defined #endif #ifdef __CUDA__ #error "Cuda should not be defined" #endif #ifde

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

jdoerfert added inline comments. Comment at: clang/lib/Headers/__clang_hip_cmath.h:96 +__DEVICE__ __CONSTEXPR__ bool isnan(float __x) { return ::__isnanf(__x); } +__DEVICE__ __CONSTEXPR__ bool isnan(double __x) { return ::__isnan(__x); } pdhaliwal wrote: > jdoe

RE: [PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

...@gmail.com; stefomeis...@gmail.com; cfe-commits@lists.llvm.org; jv...@scarletmail.rutgers.edu; zhang.guans...@gmail.com; Kumar N, Bhuvanendra ; mlek...@skidmore.edu; blitzrak...@gmail.com; shen...@google.com; t...@google.com; kuh...@google.com Subject: [PATCH] D104904: [OpenMP][AMDGCN] Initial math

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added a comment. @ronlieb reports that this change means __CUDA__ is defined for openmp amdgcn compilation. I'm going to try to verify that Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

pdhaliwal added inline comments. Comment at: clang/lib/Headers/__clang_hip_cmath.h:96 +__DEVICE__ __CONSTEXPR__ bool isnan(float __x) { return ::__isnanf(__x); } +__DEVICE__ __CONSTEXPR__ bool isnan(double __x) { return ::__isnan(__x); } jdoerfert wrote: > ^ Th

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. I recommend we ship this and fix up the rough edges as we run into them. Paired with ocml it passes OVO libm tests which seems to be a fairly high bar for 'does it work'. The

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added inline comments. Comment at: clang/lib/Headers/__clang_hip_cmath.h:43 +__DEVICE__ __CONSTEXPR__ long abs(long __n) { return ::labs(__n); } +__DEVICE__ __CONSTEXPR__ float fma(float __x, float __y, float __z) { return ::fmaf(__x, __y, __z); ---

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added inline comments. Comment at: clang/test/Headers/Inputs/include/cstdlib:29 float fabs(float __x) { return __builtin_fabs(__x); } +#endif jdoerfert wrote: > That seems to be fundamentally broken then, but let's see, maybe it will > somehow

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

jdoerfert added a comment. OpenMP side looks reasonable. Comment at: clang/lib/Headers/__clang_hip_cmath.h:96 +__DEVICE__ __CONSTEXPR__ bool isnan(float __x) { return ::__isnanf(__x); } +__DEVICE__ __CONSTEXPR__ bool isnan(double __x) { return ::__isnan(__x); } --

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

pdhaliwal added inline comments. Comment at: clang/lib/Headers/__clang_hip_cmath.h:30 +#ifdef __OPENMP_AMDGCN__ +#define __DEVICE__ static __attribute__((always_inline, nothrow)) +#define __CONSTEXPR__ constexpr ashi1 wrote: > Does OpenMP not require `__device__`

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

pdhaliwal updated this revision to Diff 354913. pdhaliwal marked 2 inline comments as done. pdhaliwal added a comment. - Move __constant__ to openmp_wrappers/cmath - Using push/pop_macro to avoid redefinition Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

ashi1 added a comment. A few small comments, otherwise LGTM on the HIP header side. Comment at: clang/lib/Headers/__clang_hip_cmath.h:30 +#ifdef __OPENMP_AMDGCN__ +#define __DEVICE__ static __attribute__((always_inline, nothrow)) +#define __CONSTEXPR__ constexpr ---

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

pdhaliwal updated this revision to Diff 354802. pdhaliwal added a comment. Typo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904 Files: clang/lib/Driver/ToolChains/Clang.cpp clang/lib/Headers/__clang_h

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

pdhaliwal added inline comments. Comment at: clang/lib/Headers/__clang_hip_math.h:29 +#else #define __DEVICE__ static __device__ inline __attribute__((always_inline)) +#endif JonChesterfield wrote: > wonder if HIP would benefit from nothrow here Would like to ke

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

pdhaliwal updated this revision to Diff 354801. pdhaliwal marked 2 inline comments as done. pdhaliwal added a comment. Addressed review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904 Files: c

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

jdoerfert added a comment. Really looking forward to this! Thanks a lot! I left some comments. Comment at: clang/lib/Headers/__clang_hip_math.h:35 +#ifdef __OPENMP_AMDGCN__ +#define __RETURN_TYPE int +#else JonChesterfield wrote: > I'd expect openmp to match t

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

tianshilei1992 added a comment. Those changes in OpenMP headers LGTM, except `#define __device__`. Comment at: clang/lib/Headers/openmp_wrappers/__clang_openmp_device_functions.h:93 + +#define __device__ __attribute__((device)) + JonChesterfield wrote: > i thi

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield added inline comments. Comment at: clang/lib/Headers/__clang_hip_cmath.h:29 +#ifdef __OPENMP_AMDGCN__ +#define __DEVICE__ static constexpr __attribute__((always_inline, nothrow)) +#define __constant__ __attribute__((constant)) scchan wrote: > `__D

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

scchan added inline comments. Comment at: clang/lib/Headers/__clang_hip_cmath.h:29 +#ifdef __OPENMP_AMDGCN__ +#define __DEVICE__ static constexpr __attribute__((always_inline, nothrow)) +#define __constant__ __attribute__((constant)) `__DEVICE__` should not imply

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

yaxunl added a comment. LGTM for HIP header changes. Pls make sure it passes internal CI (ePSDB). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904 ___ cfe-commit

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

pdhaliwal updated this revision to Diff 354471. pdhaliwal added a comment. Fix format errors Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904 Files: clang/lib/Driver/ToolChains/Clang.cpp clang/lib/Head

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

ronlieb added subscribers: estewart08, ashi1. ronlieb added a comment. @estewart08 @ashi1 please review Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904 ___ cf

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

pdhaliwal created this revision. pdhaliwal added reviewers: JonChesterfield, ye-luo, ronlieb, gregrodgers, jdoerfert. Herald added subscribers: guansong, yaxunl, jvesely. pdhaliwal requested review of this revision. Herald added subscribers: cfe-commits, sstefan1. Herald added a project: clang. W