[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. D75779 is the proper implementation of the OpenMP standard. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://reviews.llvm.org/D71179 __

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

merge_guards_bot added a comment. Build result: FAILURE - Could not check out parent git hash "9a3d576b08c13533597182498ba5e739924f892f". It was not found in the repository. Did you configure the "Parent Revision" in Phabricator properly? Trying to apply the patch to the master branch instead..

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert updated this revision to Diff 233238. jdoerfert added a comment. Fix math problem Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://reviews.llvm.org/D71179 Files: clang/include/clang/AST/Decl.h clang/include/clang/AST/

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

merge_guards_bot added a comment. Build result: FAILURE - Could not check out parent git hash "9a3d576b08c13533597182498ba5e739924f892f". It was not found in the repository. Did you configure the "Parent Revision" in Phabricator properly? Trying to apply the patch to the master branch instead..

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

merge_guards_bot added a comment. Build result: FAILURE - Could not check out parent git hash "9a3d576b08c13533597182498ba5e739924f892f". It was not found in the repository. Did you configure the "Parent Revision" in Phabricator properly? Trying to apply the patch to the master branch instead..

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert updated this revision to Diff 233232. jdoerfert added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Consistent overload based solution. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://r

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert updated this revision to Diff 233234. jdoerfert added a comment. Diff against TOT Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://reviews.llvm.org/D71179 Files: clang/include/clang/AST/Decl.h clang/include/clang/AST/

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

ABataev added a comment. In D71179#1778512 , @hfinkel wrote: > In D71179#1776761 , @ABataev wrote: > > > In D71179#1776528 , @hfinkel wrote: > > > > > In D71179#1776491

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

hfinkel added a comment. In D71179#1776761 , @ABataev wrote: > In D71179#1776528 , @hfinkel wrote: > > > In D71179#1776491 , @ABataev wrote: > > > > > In D71179#1776487

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

ABataev added a comment. In D71179#1776528 , @hfinkel wrote: > In D71179#1776491 , @ABataev wrote: > > > In D71179#1776487 , @hfinkel wrote: > > > > > In D71179#1776467

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. So, D71241 shows how declare variant (5.0) would look like if we implement it through SemaLookup. I will actually revisit this patch tomorrow as I might be able to make it even simpler. (D71241 is sav

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

hfinkel added a comment. In D71179#1776491 , @ABataev wrote: > In D71179#1776487 , @hfinkel wrote: > > > In D71179#1776467 , @ABataev wrote: > > > > > In D71179#1776457

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

ABataev added a comment. In D71179#1776487 , @hfinkel wrote: > In D71179#1776467 , @ABataev wrote: > > > In D71179#1776457 , @jdoerfert > > wrote: > > > > > > You're doing a

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

hfinkel added a comment. In D71179#1776467 , @ABataev wrote: > In D71179#1776457 , @jdoerfert wrote: > > > > You're doing absolutely the same thing as the original declare variant > > > implementation. > > > > I do

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

ABataev added a comment. In D71179#1776457 , @jdoerfert wrote: > > You're doing absolutely the same thing as the original declare variant > > implementation. > > I don't think so but if you do why do you oppose this approach? > > > And I don't think it wo

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. > You're doing absolutely the same thing as the original declare variant > implementation. I don't think so but if you do why do you oppose this approach? > And I don't think it would be correct to add them as multiversiin variants to > the original function. Why wo

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: clang/lib/Headers/__clang_cuda_math_forward_declares.h:41 __DEVICE__ long abs(long); __DEVICE__ long long abs(long long); -__DEVICE__ double abs(double); I have to double c

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

ABataev added a comment. In D71179#1776108 , @jdoerfert wrote: > In D71179#1776046 , @ABataev wrote: > > > In D71179#1776034 , @jdoerfert > > wrote: > > > > > > Not always.

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. In D71179#1776046 , @ABataev wrote: > In D71179#1776034 , @jdoerfert wrote: > > > > Not always. If we see that the context selector does not match, we can > > > skip everything between be

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

ABataev added a comment. In D71179#1776034 , @jdoerfert wrote: > > Not always. If we see that the context selector does not match, we can skip > > everything between begin/end. It means exactly what I said - > > multiversioning is needed only for `constr

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. > Not always. If we see that the context selector does not match, we can skip > everything between begin/end. It means exactly what I said - multiversioning > is needed only for `construct` because all other traits can be easily > resolved at the compile time. General

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

ABataev added a comment. In D71179#1775834 , @jdoerfert wrote: > >> This is neither true, nor relevant. It is not true because OpenMP 5.0 > >> declare variant is so broken it cannot be used for what it was intended > >> for. That means people (as for exa

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

merge_guards_bot added a comment. Build result: fail - 60639 tests passed, 24 failed and 726 were skipped. failed: Clang.CXX/dcl_dcl/basic_namespace/namespace_udecl/p11.cpp failed: Clang.CXX/drs/dr5xx.cpp failed: Clang.CXX/modules-ts/basic/basic_def_odr/p6/global-vs-module.cpp failed: Cla

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. >> This is neither true, nor relevant. It is not true because OpenMP 5.0 >> declare variant is so broken it cannot be used for what it was intended for. >> That means people (as for example we for math) will inevitably use begin/end >> declare variant. > > I rather d

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert updated this revision to Diff 232909. jdoerfert marked 6 inline comments as done. jdoerfert added a comment. Undo math function removal (fpclassify & scalblnf), reorder includes (host first) The latter is the "natural way" but also necessary because fpclassify uses macros and we did not

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added inline comments. Comment at: clang/lib/Headers/__clang_cuda_cmath.h:72 -#ifndef _OPENMP -__DEVICE__ int fpclassify(float __x) { - return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL, FP_SUBNORMAL, tra wrote: > Please keep fpclassify in pla

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

ABataev added a comment. In D71179#1775687 , @jdoerfert wrote: > > @jdoerfert , also, do we have tests that can go into the test suite / > > libomptarget regression tests demonstrating the collection of problems > > people have currently opened bugs on r

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

merge_guards_bot added a comment. Build result: fail - 60641 tests passed, 20 failed and 726 were skipped. failed: Clang.CXX/dcl_dcl/basic_namespace/namespace_udecl/p11.cpp failed: Clang.CXX/drs/dr5xx.cpp failed: Clang.CXX/modules-ts/basic/basic_def_odr/p6/global-vs-module.cpp failed: Cla

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert updated this revision to Diff 232906. jdoerfert added a comment. minor fix Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://reviews.llvm.org/D71179 Files: clang/include/clang/AST/Decl.h clang/include/clang/AST/StmtOpe

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

merge_guards_bot added a comment. Build result: fail - 60637 tests passed, 24 failed and 726 were skipped. failed: Clang.CXX/dcl_dcl/basic_namespace/namespace_udecl/p11.cpp failed: Clang.CXX/drs/dr5xx.cpp failed: Clang.CXX/modules-ts/basic/basic_def_odr/p6/global-vs-module.cpp failed: Cla

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

tra added inline comments. Comment at: clang/lib/Headers/__clang_cuda_cmath.h:72 -#ifndef _OPENMP -__DEVICE__ int fpclassify(float __x) { - return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL, FP_SUBNORMAL, Please keep fpclassify in place. It's been avail

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

hfinkel added a comment. In D71179#1775687 , @jdoerfert wrote: > ... > > >>> We restricted it for now to function definitions so we don't need to define >>> the mangling as you cannot expect linking. (I did this to get it in TR8 >>> while I figured

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert updated this revision to Diff 232902. jdoerfert added a comment. Add one more test sin(long double), and fix some rebase issues Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://reviews.llvm.org/D71179 Files: clang/inclu

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. > @jdoerfert , also, do we have tests that can go into the test suite / > libomptarget regression tests demonstrating the collection of problems people > have currently opened bugs on regarding math.h? I recall we still had > problems with host code needing the long-d

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

hfinkel added a comment. In D71179#1775442 , @jdoerfert wrote: > > @jdoerfert , how does the ".ompvariant" work with external functions? I see > > the part of the spec which says, "The symbol name of a function definition > > that appears between a begin

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

ABataev added a comment. In D71179#1775442 , @jdoerfert wrote: > > @jdoerfert , how does the ".ompvariant" work with external functions? I see > > the part of the spec which says, "The symbol name of a function definition > > that appears between a begin

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert marked 5 inline comments as done. jdoerfert added a comment. > @jdoerfert , how does the ".ompvariant" work with external functions? I see > the part of the spec which says, "The symbol name of a function definition > that appears between a begin declare variant...", but, if we append

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

hfinkel added a comment. In D71179#1775157 , @ABataev wrote: > In D71179#1775066 , @hfinkel wrote: > > > In D71179#1774678 , @ABataev wrote: > > > > > In D71179#1774487

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert marked 11 inline comments as done. jdoerfert added inline comments. Comment at: clang/lib/AST/StmtOpenMP.cpp:2243 } + +// TODO: We have various representations for the same data, it might help to ABataev wrote: > jdoerfert wrote: > > This code was basi

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

ABataev added a comment. In D71179#1775066 , @hfinkel wrote: > In D71179#1774678 , @ABataev wrote: > > > In D71179#1774487 , @jdoerfert > > wrote: > > > > > In D71179#177447

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

hfinkel added a comment. In D71179#1774678 , @ABataev wrote: > In D71179#1774487 , @jdoerfert wrote: > > > In D71179#1774471 , @ABataev wrote: > > > > > They do this because

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

JonChesterfield added inline comments. Comment at: clang/lib/Headers/__clang_cuda_cmath.h:70 __DEVICE__ float fmod(float __x, float __y) { return ::fmodf(__x, __y); } -// TODO: remove when variant is supported -#ifndef _OPENMP jdoerfert wrote: > As far as I can

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

JonChesterfield added inline comments. Comment at: clang/test/OpenMP/begin_declare_variant_codegen.cpp:71 +} + +// Make sure all ompvariant functions return 1 and all others return 0. The name mangling should probably append the device kind, .e.g. `_Z3foov.ompva

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

ABataev added a comment. In D71179#1774487 , @jdoerfert wrote: > In D71179#1774471 , @ABataev wrote: > > > They do this because they have several function definitions with the same > > name. In our case, we have se

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

JonChesterfield added a comment. Great to see the fragile math.h stuff disappear. I'm not sure about the CPU/GPU/other granularity. An openmp program with x86 as the host and target offload regions for amdgcn and for nvptx seems like a reasonable aspiration. Or for a couple of different generat

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

hfinkel added a comment. In D71179#1774487 , @jdoerfert wrote: > In D71179#1774471 , @ABataev wrote: > > > They do this because they have several function definitions with the same > > name. In our case, we have se

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

hfinkel added inline comments. Comment at: clang/lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h:17 #if defined(__NVPTX__) && defined(_OPENMP) Should we use a more-specific selector and then get rid of this `__NVPTX__` check? C

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. In D71179#1774471 , @ABataev wrote: > They do this because they have several function definitions with the same > name. In our case, we have several different functions with different names > and for us no need to worry about o

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

ABataev added a comment. In D71179#1774470 , @jdoerfert wrote: > In D71179#1774469 , @ABataev wrote: > > > In D71179#1774448 , @jdoerfert > > wrote: > > > > > In D71179#1774

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. In D71179#1774469 , @ABataev wrote: > In D71179#1774448 , @jdoerfert wrote: > > > In D71179#177 , @ABataev wrote: > > > > > I read the spec and

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

ABataev added a comment. In D71179#1774448 , @jdoerfert wrote: > In D71179#177 , @ABataev wrote: > > > I read the spec and don't think that we need all this complex stuff for the > > implementation. Yiu need ju

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. In D71179#177 , @ABataev wrote: > I read the spec and don't think that we need all this complex stuff for the > implementation. Yiu need judt to check at the codegen phase if the function > must be emitted or not. We don't

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

ABataev added a comment. I read the spec and don't think that we need all this complex stuff for the implementation. Yiu need judt to check at the codegen phase if the function must be emitted or not. We don't even need to move context checksnfrom codegen, because with the current semantics all

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

merge_guards_bot added a comment. Build result: FAILURE - Log files: console-log.txt , CMakeCache.txt Repository: rG LLV

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert marked 4 inline comments as done. jdoerfert added inline comments. Comment at: clang/lib/AST/StmtOpenMP.cpp:2243 } + +// TODO: We have various representations for the same data, it might help to This code was basically only moved, not written for this

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert updated this revision to Diff 232750. jdoerfert added a comment. Add (missing) include. (Worked locally just fine). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://reviews.llvm.org/D71179 Files: clang/include/clang/AST

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

merge_guards_bot added a comment. Build result: FAILURE - Log files: console-log.txt , CMakeCache.txt Repository: rG LLV

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert created this revision. jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim. Herald added subscribers: s.egerton, guansong, bollu, simoncook, fedor.sergeev, aheejin, rampitec. Herald added a project: cla