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

[Johannes Doerfert via Phabricator via cfe-commits]

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



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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

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...

ERROR: arc patch failed with error code 1. Check build log for details.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.h
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Overload.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Headers/__clang_cuda_cmath.h
  clang/lib/Headers/__clang_cuda_device_functions.h
  clang/lib/Headers/__clang_cuda_math_forward_declares.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  clang/lib/Headers/openmp_wrappers/cmath
  clang/lib/Headers/openmp_wrappers/math.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant.c
  clang/test/OpenMP/begin_declare_variant_codegen.cpp
  clang/test/OpenMP/declare_variant_ast_print.cpp
  clang/test/OpenMP/math_codegen.cpp
  clang/test/OpenMP/math_fp_macro.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def

Index: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
===
--- llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
+++ llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
@@ -91,6 +91,8 @@
 __OMP_DIRECTIVE_EXT(master_taskloop_simd, "master taskloop simd")
 __OMP_DIRECTIVE_EXT(parallel_master_taskloop_simd,
 "parallel master taskloop simd")
+__OMP_DIRECTIVE_EXT(begin_declare_variant, "begin declare variant")
+__OMP_DIRECTIVE_EXT(end_declare_variant, "end declare variant")
 
 // Has to be the last because Clang implicitly expects it to be.
 __OMP_DIRECTIVE(unknown)
Index: clang/test/OpenMP/math_fp_macro.cpp
===
--- /dev/null
+++ clang/test/OpenMP/math_fp_macro.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+#include 
+
+int main() {
+  double a(0);
+  return (std::fpclassify(a) != FP_ZERO);
+}
Index: clang/test/OpenMP/math_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/math_codegen.cpp
@@ -0,0 +1,15 @@
+#include 
+
+void math(short s, int i, float f, double d) {
+  sin(s);
+  sin(i);
+  sin(f);
+  sin(d);
+}
+
+void foo(short s, int i, float f, double d, long double ld) {
+  //sin(ld);
+  math(s, i, f, d);
+#pragma omp target
+  { math(s, i, f, d); }
+}
Index: clang/test/OpenMP/declare_variant_ast_print.cpp
===
--- clang/test/OpenMP/declare_variant_ast_print.cpp
+++ clang/test/OpenMP/declare_variant_ast_print.cpp
@@ -40,7 +40,6 @@
 #pragma omp declare variant(foofoo ) match(user = {condition()})
 #pragma omp declare variant(foofoo ) match(implementation={vendor(llvm)},device={kind(cpu)})
 #pragma omp declare variant(foofoo ) match(implementation={vendor(unknown)})
-// TODO: Handle template instantiation
 #pragma omp declare variant(foofoo ) match(implementation={vendor(score(C+5): ibm, xxx, ibm)},device={kind(cpu,host)})
 template 
 T barbar();
Index: clang/test/OpenMP/begin_declare_variant_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/begin_declare_variant_codegen.cpp
@@ -0,0 +1,134 @@
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+int bar(void) {
+  return 0;
+}
+
+template 
+T baz(void) { return 0; }
+
+#pragma omp begin declare variant match(device={kind(cpu)})
+int foo(void) {
+  return 1;
+}
+int bar(void) {
+  return 1;
+}
+template 
+T baz(void) { return 1; }
+
+template 
+T biz(void) { return 1; }
+
+template 
+T buz(void) { return 3; }
+
+template <>
+char buz(void) { return 1; }
+
+template 
+T bez(void) { return 3; }
+#pragma omp end declare variant
+
+#pragma omp begin declare variant match(device={kind(gpu)})
+int foo(void) {
+  return 2;
+}
+int bar(void) {
+  return 2;
+}
+#pragma omp end declare variant
+
+
+#pragma omp begin declare variant match(device={kind(fpga)})
+
+This text is never parsed!
+
+#pragma omp end declare variant
+
+int foo(void) {
+  return 0;
+}
+
+template 
+T biz(void) { return 0; }
+
+template <>
+char buz(void) { return 0; }
+
+template <>
+long bez(void) { return 0; }
+
+#pragma omp begin 

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

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...

ERROR: arc patch failed with error code 1. Check build log for details.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

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...

ERROR: arc patch failed with error code 1. Check build log for details.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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://reviews.llvm.org/D71179

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.h
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Overload.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Headers/__clang_cuda_cmath.h
  clang/lib/Headers/__clang_cuda_device_functions.h
  clang/lib/Headers/__clang_cuda_math_forward_declares.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  clang/lib/Headers/openmp_wrappers/cmath
  clang/lib/Headers/openmp_wrappers/math.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant.c
  clang/test/OpenMP/begin_declare_variant_codegen.cpp
  clang/test/OpenMP/declare_variant_ast_print.cpp
  clang/test/OpenMP/math_codegen.cpp
  clang/test/OpenMP/math_fp_macro.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def

Index: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
===
--- llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
+++ llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
@@ -91,6 +91,8 @@
 __OMP_DIRECTIVE_EXT(master_taskloop_simd, "master taskloop simd")
 __OMP_DIRECTIVE_EXT(parallel_master_taskloop_simd,
 "parallel master taskloop simd")
+__OMP_DIRECTIVE_EXT(begin_declare_variant, "begin declare variant")
+__OMP_DIRECTIVE_EXT(end_declare_variant, "end declare variant")
 
 // Has to be the last because Clang implicitly expects it to be.
 __OMP_DIRECTIVE(unknown)
Index: clang/test/OpenMP/math_fp_macro.cpp
===
--- /dev/null
+++ clang/test/OpenMP/math_fp_macro.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+#include 
+
+int main() {
+  double a(0);
+  return (std::fpclassify(a) != FP_ZERO);
+}
Index: clang/test/OpenMP/math_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/math_codegen.cpp
@@ -0,0 +1,15 @@
+#include 
+
+void math(short s, int i, float f, double d) {
+  sin(s);
+  sin(i);
+  sin(f);
+  sin(d);
+}
+
+void foo(short s, int i, float f, double d, long double ld) {
+  //sin(ld);
+  math(s, i, f, d);
+#pragma omp target
+  { math(s, i, f, d); }
+}
Index: clang/test/OpenMP/declare_variant_ast_print.cpp
===
--- clang/test/OpenMP/declare_variant_ast_print.cpp
+++ clang/test/OpenMP/declare_variant_ast_print.cpp
@@ -40,7 +40,6 @@
 #pragma omp declare variant(foofoo ) match(user = {condition()})
 #pragma omp declare variant(foofoo ) match(implementation={vendor(llvm)},device={kind(cpu)})
 #pragma omp declare variant(foofoo ) match(implementation={vendor(unknown)})
-// TODO: Handle template instantiation
 #pragma omp declare variant(foofoo ) match(implementation={vendor(score(C+5): ibm, xxx, ibm)},device={kind(cpu,host)})
 template 
 T barbar();
Index: clang/test/OpenMP/begin_declare_variant_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/begin_declare_variant_codegen.cpp
@@ -0,0 +1,134 @@
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+int bar(void) {
+  return 0;
+}
+
+template 
+T baz(void) { return 0; }
+
+#pragma omp begin declare variant match(device={kind(cpu)})
+int foo(void) {
+  return 1;
+}
+int bar(void) {
+  return 1;
+}
+template 
+T baz(void) { return 1; }
+
+template 
+T biz(void) { return 1; }
+
+template 
+T buz(void) { return 3; }
+
+template <>
+char buz(void) { return 1; }
+
+template 
+T bez(void) { return 3; }
+#pragma omp end declare variant
+
+#pragma omp begin declare variant match(device={kind(gpu)})
+int foo(void) {
+  return 2;
+}
+int bar(void) {
+  return 2;
+}
+#pragma omp end declare variant
+
+
+#pragma omp begin declare variant match(device={kind(fpga)})
+
+This text is never parsed!
+
+#pragma omp end declare variant
+
+int foo(void) {
+  return 0;
+}
+
+template 
+T biz(void) { return 0; }
+
+template <>
+char 

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

[Johannes Doerfert via Phabricator via cfe-commits]

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/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.h
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Overload.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Headers/__clang_cuda_cmath.h
  clang/lib/Headers/__clang_cuda_device_functions.h
  clang/lib/Headers/__clang_cuda_math_forward_declares.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  clang/lib/Headers/openmp_wrappers/cmath
  clang/lib/Headers/openmp_wrappers/math.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant.c
  clang/test/OpenMP/begin_declare_variant_codegen.cpp
  clang/test/OpenMP/declare_variant_ast_print.cpp
  clang/test/OpenMP/math_codegen.cpp
  clang/test/OpenMP/math_fp_macro.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def

Index: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
===
--- llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
+++ llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
@@ -91,6 +91,8 @@
 __OMP_DIRECTIVE_EXT(master_taskloop_simd, "master taskloop simd")
 __OMP_DIRECTIVE_EXT(parallel_master_taskloop_simd,
 "parallel master taskloop simd")
+__OMP_DIRECTIVE_EXT(begin_declare_variant, "begin declare variant")
+__OMP_DIRECTIVE_EXT(end_declare_variant, "end declare variant")
 
 // Has to be the last because Clang implicitly expects it to be.
 __OMP_DIRECTIVE(unknown)
Index: clang/test/OpenMP/math_fp_macro.cpp
===
--- /dev/null
+++ clang/test/OpenMP/math_fp_macro.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+#include 
+
+int main() {
+  double a(0);
+  return (std::fpclassify(a) != FP_ZERO);
+}
Index: clang/test/OpenMP/math_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/math_codegen.cpp
@@ -0,0 +1,15 @@
+#include 
+
+void math(short s, int i, float f, double d) {
+  sin(s);
+  sin(i);
+  sin(f);
+  sin(d);
+}
+
+void foo(short s, int i, float f, double d, long double ld) {
+  //sin(ld);
+  math(s, i, f, d);
+#pragma omp target
+  { math(s, i, f, d); }
+}
Index: clang/test/OpenMP/declare_variant_ast_print.cpp
===
--- clang/test/OpenMP/declare_variant_ast_print.cpp
+++ clang/test/OpenMP/declare_variant_ast_print.cpp
@@ -40,7 +40,6 @@
 #pragma omp declare variant(foofoo ) match(user = {condition()})
 #pragma omp declare variant(foofoo ) match(implementation={vendor(llvm)},device={kind(cpu)})
 #pragma omp declare variant(foofoo ) match(implementation={vendor(unknown)})
-// TODO: Handle template instantiation
 #pragma omp declare variant(foofoo ) match(implementation={vendor(score(C+5): ibm, xxx, ibm)},device={kind(cpu,host)})
 template 
 T barbar();
Index: clang/test/OpenMP/begin_declare_variant_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/begin_declare_variant_codegen.cpp
@@ -0,0 +1,134 @@
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+int bar(void) {
+  return 0;
+}
+
+template 
+T baz(void) { return 0; }
+
+#pragma omp begin declare variant match(device={kind(cpu)})
+int foo(void) {
+  return 1;
+}
+int bar(void) {
+  return 1;
+}
+template 
+T baz(void) { return 1; }
+
+template 
+T biz(void) { return 1; }
+
+template 
+T buz(void) { return 3; }
+
+template <>
+char buz(void) { return 1; }
+
+template 
+T bez(void) { return 3; }
+#pragma omp end declare variant
+
+#pragma omp begin declare variant match(device={kind(gpu)})
+int foo(void) {
+  return 2;
+}
+int bar(void) {
+  return 2;
+}
+#pragma omp end declare variant
+
+
+#pragma omp begin declare variant match(device={kind(fpga)})
+
+This text is never parsed!
+
+#pragma omp end declare variant
+
+int foo(void) {
+  return 0;
+}
+
+template 
+T biz(void) { return 0; }
+
+template <>
+char buz(void) { return 0; }
+
+template <>
+long bez(void) { return 0; }
+
+#pragma omp begin 

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D71179#1778512 , @hfinkel wrote:

> In D71179#1776761 , @ABataev wrote:
>
> > In D71179#1776528 , @hfinkel wrote:
> >
> > > In D71179#1776491 , @ABataev 
> > > wrote:
> > >
> > > > In D71179#1776487 , @hfinkel 
> > > > wrote:
> > > >
> > > > > In D71179#1776467 , @ABataev 
> > > > > wrote:
> > > > >
> > > > > > 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 would be correct to add them as 
> > > > > > > > multiversiin variants to the original function.
> > > > > > >
> > > > > > > Why wouldn't it be correct to pick the version through the 
> > > > > > > overload resolution instead of the code generation?
> > > > > > >  How this could work is already described in the TODO 
> > > > > > > (CodeGenModule.cpp):
> > > > > > >
> > > > > > >   // TODO: We should introduce function aliases for `omp declare 
> > > > > > > variant`
> > > > > > >   //   directives such that we can treat them through the 
> > > > > > > same overload
> > > > > > >   //   resolution scheme (via multi versioning) as `omp begin 
> > > > > > > declare
> > > > > > >   //   variant` functions. For an `omp declare 
> > > > > > > variant(VARIANT) ...`
> > > > > > >   //   that is attached to a BASE function we would create a 
> > > > > > > global alias
> > > > > > >   //   VARIANT = BASE which will participate in the multi 
> > > > > > > version overload
> > > > > > >   //   resolution. If picked, here is no need to emit them 
> > > > > > > explicitly.
> > > > > > >   
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >  ---
> > > > > > >
> > > > > > > I still haven't understood why we cannot/should not reuse the 
> > > > > > > existing multi-version support and instead duplicate the logic in 
> > > > > > > some custom scheme.
> > > > > > >  We have this patch that shows how we can reuse the logic in 
> > > > > > > Clang. It works on a per-call basis, so it will work for all 
> > > > > > > context selector (incl. construct).
> > > > > > >  If you think there is something conceptually not working, I'd 
> > > > > > > like to hear about it. However, just saying "it wouldn't be 
> > > > > > > correct" is not sufficient. You need to provide details about the 
> > > > > > > situation, what you think would not work, and why.
> > > > > >
> > > > > >
> > > > > > I explayned already: declare variant cannot be represented as 
> > > > > > mutiversion functiin, for example.
> > > > >
> > > > >
> > > > > @ABataev, can you please elaborate? It's not obvious to me that we 
> > > > > cannot handle the existing declare variant with the same scheme (as 
> > > > > @jdoerfert highlighted above). In general, I believe it's preferable 
> > > > > to have one generic scheme and use it to handle all cases as opposed 
> > > > > to continuing to use a more-limited scheme in addition to the generic 
> > > > > scheme.
> > > >
> > > >
> > > > Eaine already. Current version of declare variant cannot be represented 
> > > > as multiversiin functions, because it is not. We have a function that 
> > > > is the alias to other functions with different names. They just are not 
> > > > multiversion functions by definition.
> > >
> > >
> > > I understand that they have different names. I don't see why we that 
> > > means that they can't be added to the overload set as multi-version 
> > > candidates if we add logic which does exactly that.
> >
> >
> >
>
>
> @jdoerfert posted a prototype implementation in D71241 
> , so we don't need to just have a 
> theoretical discussion, but I'd like to address a high-level issue here:
>
> > Because this is exactly what I said- you want to reuse the exiwting 
> > solution for completely different purpose just because you want to you 
> > reuse though even semantically it has nothing to do with multiversioning.
>
> This kind of comment really isn't appropriate. We're all experienced 
> developers here, and no one is proposing to reuse code in an inappropriate 
> manner "just because" or for any other reason. I ask you to reconsider your 
> reasoning here for two reasons:
>
> 1. "Reus[ing] the existing solution for a completely different purpose", 
> which I'll classify as structural code reuse, is not necessarily bad. 
> Structural code reuse, where you reuse code with a similar structure, but 
> different purpose, from what you need, is often a useful impetus for the 
> creation of new abstractions. 

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

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D71179#1776761 , @ABataev wrote:

> In D71179#1776528 , @hfinkel wrote:
>
> > In D71179#1776491 , @ABataev wrote:
> >
> > > In D71179#1776487 , @hfinkel 
> > > wrote:
> > >
> > > > In D71179#1776467 , @ABataev 
> > > > wrote:
> > > >
> > > > > 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 would be correct to add them as multiversiin 
> > > > > > > variants to the original function.
> > > > > >
> > > > > > Why wouldn't it be correct to pick the version through the overload 
> > > > > > resolution instead of the code generation?
> > > > > >  How this could work is already described in the TODO 
> > > > > > (CodeGenModule.cpp):
> > > > > >
> > > > > >   // TODO: We should introduce function aliases for `omp declare 
> > > > > > variant`
> > > > > >   //   directives such that we can treat them through the same 
> > > > > > overload
> > > > > >   //   resolution scheme (via multi versioning) as `omp begin 
> > > > > > declare
> > > > > >   //   variant` functions. For an `omp declare variant(VARIANT) 
> > > > > > ...`
> > > > > >   //   that is attached to a BASE function we would create a 
> > > > > > global alias
> > > > > >   //   VARIANT = BASE which will participate in the multi 
> > > > > > version overload
> > > > > >   //   resolution. If picked, here is no need to emit them 
> > > > > > explicitly.
> > > > > >   
> > > > > >
> > > > > >
> > > > > >
> > > > > >  ---
> > > > > >
> > > > > > I still haven't understood why we cannot/should not reuse the 
> > > > > > existing multi-version support and instead duplicate the logic in 
> > > > > > some custom scheme.
> > > > > >  We have this patch that shows how we can reuse the logic in Clang. 
> > > > > > It works on a per-call basis, so it will work for all context 
> > > > > > selector (incl. construct).
> > > > > >  If you think there is something conceptually not working, I'd like 
> > > > > > to hear about it. However, just saying "it wouldn't be correct" is 
> > > > > > not sufficient. You need to provide details about the situation, 
> > > > > > what you think would not work, and why.
> > > > >
> > > > >
> > > > > I explayned already: declare variant cannot be represented as 
> > > > > mutiversion functiin, for example.
> > > >
> > > >
> > > > @ABataev, can you please elaborate? It's not obvious to me that we 
> > > > cannot handle the existing declare variant with the same scheme (as 
> > > > @jdoerfert highlighted above). In general, I believe it's preferable to 
> > > > have one generic scheme and use it to handle all cases as opposed to 
> > > > continuing to use a more-limited scheme in addition to the generic 
> > > > scheme.
> > >
> > >
> > > Eaine already. Current version of declare variant cannot be represented 
> > > as multiversiin functions, because it is not. We have a function that is 
> > > the alias to other functions with different names. They just are not 
> > > multiversion functions by definition.
> >
> >
> > I understand that they have different names. I don't see why we that means 
> > that they can't be added to the overload set as multi-version candidates if 
> > we add logic which does exactly that.
>
>
>


@jdoerfert posted a prototype implementation in D71241 
, so we don't need to just have a theoretical 
discussion, but I'd like to address a high-level issue here:

> Because this is exactly what I said- you want to reuse the exiwting solution 
> for completely different purpose just because you want to you reuse though 
> even semantically it has nothing to do with multiversioning.

This kind of comment really isn't appropriate. We're all experienced developers 
here, and no one is proposing to reuse code in an inappropriate manner "just 
because" or for any other reason. I ask you to reconsider your reasoning here 
for two reasons:

1. "Reus[ing] the existing solution for a completely different purpose", which 
I'll classify as structural code reuse, is not necessarily bad. Structural code 
reuse, where you reuse code with a similar structure, but different purpose, 
from what you need, is often a useful impetus for the creation of new 
abstractions. The trade off relevant here, in my experience, is against future 
structural divergence. In the future, is it likely that the abstraction will 
break down because the different purposes will tend to require the code 
structure to change in the future in divergent ways? If so, that can be 

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D71179#1776528 , @hfinkel wrote:

> In D71179#1776491 , @ABataev wrote:
>
> > In D71179#1776487 , @hfinkel wrote:
> >
> > > In D71179#1776467 , @ABataev 
> > > wrote:
> > >
> > > > 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 would be correct to add them as multiversiin 
> > > > > > variants to the original function.
> > > > >
> > > > > Why wouldn't it be correct to pick the version through the overload 
> > > > > resolution instead of the code generation?
> > > > >  How this could work is already described in the TODO 
> > > > > (CodeGenModule.cpp):
> > > > >
> > > > >   // TODO: We should introduce function aliases for `omp declare 
> > > > > variant`
> > > > >   //   directives such that we can treat them through the same 
> > > > > overload
> > > > >   //   resolution scheme (via multi versioning) as `omp begin 
> > > > > declare
> > > > >   //   variant` functions. For an `omp declare variant(VARIANT) 
> > > > > ...`
> > > > >   //   that is attached to a BASE function we would create a 
> > > > > global alias
> > > > >   //   VARIANT = BASE which will participate in the multi version 
> > > > > overload
> > > > >   //   resolution. If picked, here is no need to emit them 
> > > > > explicitly.
> > > > >   
> > > > >
> > > > >
> > > > >
> > > > >  ---
> > > > >
> > > > > I still haven't understood why we cannot/should not reuse the 
> > > > > existing multi-version support and instead duplicate the logic in 
> > > > > some custom scheme.
> > > > >  We have this patch that shows how we can reuse the logic in Clang. 
> > > > > It works on a per-call basis, so it will work for all context 
> > > > > selector (incl. construct).
> > > > >  If you think there is something conceptually not working, I'd like 
> > > > > to hear about it. However, just saying "it wouldn't be correct" is 
> > > > > not sufficient. You need to provide details about the situation, what 
> > > > > you think would not work, and why.
> > > >
> > > >
> > > > I explayned already: declare variant cannot be represented as 
> > > > mutiversion functiin, for example.
> > >
> > >
> > > @ABataev, can you please elaborate? It's not obvious to me that we cannot 
> > > handle the existing declare variant with the same scheme (as @jdoerfert 
> > > highlighted above). In general, I believe it's preferable to have one 
> > > generic scheme and use it to handle all cases as opposed to continuing to 
> > > use a more-limited scheme in addition to the generic scheme.
> >
> >
> > Eaine already. Current version of declare variant cannot be represented as 
> > multiversiin functions, because it is not. We have a function that is the 
> > alias to other functions with different names. They just are not 
> > multiversion functions by definition.
>
>
> I understand that they have different names. I don't see why we that means 
> that they can't be added to the overload set as multi-version candidates if 
> we add logic which does exactly that.


Because this is exactly what I said- you want to reuse the exiwting solution 
for completely different purpose just because you want to you reuse though even 
semantically it has nothing to do with multiversioning. And I think it is bad 
idead to break the semantics of the existing solution. It requires some 
addition changes like merging of different functiins with different names. And 
here I want to ask - why do you think it is better than my proposal to reuse 
the codegen for the already implemented declare variant stuff for the OpenMP  
multiversioned functions? It really requires less work, bdcause you just need 
to add a loop over all varinants and call `tryEmit...` function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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 saving ~250 lines and from what I've seen 
in the tests actually fixing things.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D71179#1776491 , @ABataev wrote:

> In D71179#1776487 , @hfinkel wrote:
>
> > In D71179#1776467 , @ABataev wrote:
> >
> > > 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 would be correct to add them as multiversiin 
> > > > > variants to the original function.
> > > >
> > > > Why wouldn't it be correct to pick the version through the overload 
> > > > resolution instead of the code generation?
> > > >  How this could work is already described in the TODO 
> > > > (CodeGenModule.cpp):
> > > >
> > > >   // TODO: We should introduce function aliases for `omp declare 
> > > > variant`
> > > >   //   directives such that we can treat them through the same 
> > > > overload
> > > >   //   resolution scheme (via multi versioning) as `omp begin 
> > > > declare
> > > >   //   variant` functions. For an `omp declare variant(VARIANT) ...`
> > > >   //   that is attached to a BASE function we would create a global 
> > > > alias
> > > >   //   VARIANT = BASE which will participate in the multi version 
> > > > overload
> > > >   //   resolution. If picked, here is no need to emit them 
> > > > explicitly.
> > > >   
> > > >
> > > >
> > > >
> > > >  ---
> > > >
> > > > I still haven't understood why we cannot/should not reuse the existing 
> > > > multi-version support and instead duplicate the logic in some custom 
> > > > scheme.
> > > >  We have this patch that shows how we can reuse the logic in Clang. It 
> > > > works on a per-call basis, so it will work for all context selector 
> > > > (incl. construct).
> > > >  If you think there is something conceptually not working, I'd like to 
> > > > hear about it. However, just saying "it wouldn't be correct" is not 
> > > > sufficient. You need to provide details about the situation, what you 
> > > > think would not work, and why.
> > >
> > >
> > > I explayned already: declare variant cannot be represented as mutiversion 
> > > functiin, for example.
> >
> >
> > @ABataev, can you please elaborate? It's not obvious to me that we cannot 
> > handle the existing declare variant with the same scheme (as @jdoerfert 
> > highlighted above). In general, I believe it's preferable to have one 
> > generic scheme and use it to handle all cases as opposed to continuing to 
> > use a more-limited scheme in addition to the generic scheme.
>
>
> Eaine already. Current version of declare variant cannot be represented as 
> multiversiin functions, because it is not. We have a function that is the 
> alias to other functions with different names. They just are not multiversion 
> functions by definition.


I understand that they have different names. I don't see why we that means that 
they can't be added to the overload set as multi-version candidates if we add 
logic which does exactly that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D71179#1776487 , @hfinkel wrote:

> In D71179#1776467 , @ABataev wrote:
>
> > 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 would be correct to add them as multiversiin 
> > > > variants to the original function.
> > >
> > > Why wouldn't it be correct to pick the version through the overload 
> > > resolution instead of the code generation?
> > >  How this could work is already described in the TODO (CodeGenModule.cpp):
> > >
> > >   // TODO: We should introduce function aliases for `omp declare variant`
> > >   //   directives such that we can treat them through the same 
> > > overload
> > >   //   resolution scheme (via multi versioning) as `omp begin declare
> > >   //   variant` functions. For an `omp declare variant(VARIANT) ...`
> > >   //   that is attached to a BASE function we would create a global 
> > > alias
> > >   //   VARIANT = BASE which will participate in the multi version 
> > > overload
> > >   //   resolution. If picked, here is no need to emit them explicitly.
> > >   
> > >
> > >
> > >
> > >  ---
> > >
> > > I still haven't understood why we cannot/should not reuse the existing 
> > > multi-version support and instead duplicate the logic in some custom 
> > > scheme.
> > >  We have this patch that shows how we can reuse the logic in Clang. It 
> > > works on a per-call basis, so it will work for all context selector 
> > > (incl. construct).
> > >  If you think there is something conceptually not working, I'd like to 
> > > hear about it. However, just saying "it wouldn't be correct" is not 
> > > sufficient. You need to provide details about the situation, what you 
> > > think would not work, and why.
> >
> >
> > I explayned already: declare variant cannot be represented as mutiversion 
> > functiin, for example.
>
>
> @ABataev, can you please elaborate? It's not obvious to me that we cannot 
> handle the existing declare variant with the same scheme (as @jdoerfert 
> highlighted above). In general, I believe it's preferable to have one generic 
> scheme and use it to handle all cases as opposed to continuing to use a 
> more-limited scheme in addition to the generic scheme.


Eaine already. Current version of declare variant cannot be represented as 
multiversiin functions, because it is not. We have a function that is the alias 
to other functions with different names. They just are not multiversion 
functions by definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Hal Finkel via Phabricator via cfe-commits]

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 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 wouldn't it be correct to pick the version through the overload 
> > resolution instead of the code generation?
> >  How this could work is already described in the TODO (CodeGenModule.cpp):
> >
> >   // TODO: We should introduce function aliases for `omp declare variant`
> >   //   directives such that we can treat them through the same overload
> >   //   resolution scheme (via multi versioning) as `omp begin declare
> >   //   variant` functions. For an `omp declare variant(VARIANT) ...`
> >   //   that is attached to a BASE function we would create a global 
> > alias
> >   //   VARIANT = BASE which will participate in the multi version 
> > overload
> >   //   resolution. If picked, here is no need to emit them explicitly.
> >   
> >
> >
> >
> >  ---
> >
> > I still haven't understood why we cannot/should not reuse the existing 
> > multi-version support and instead duplicate the logic in some custom scheme.
> >  We have this patch that shows how we can reuse the logic in Clang. It 
> > works on a per-call basis, so it will work for all context selector (incl. 
> > construct).
> >  If you think there is something conceptually not working, I'd like to hear 
> > about it. However, just saying "it wouldn't be correct" is not sufficient. 
> > You need to provide details about the situation, what you think would not 
> > work, and why.
>
>
> I explayned already: declare variant cannot be represented as mutiversion 
> functiin, for example.


@ABataev, can you please elaborate? It's not obvious to me that we cannot 
handle the existing declare variant with the same scheme (as @jdoerfert 
highlighted above). In general, I believe it's preferable to have one generic 
scheme and use it to handle all cases as opposed to continuing to use a 
more-limited scheme in addition to the generic scheme.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Alexey Bataev via Phabricator via cfe-commits]

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 would be correct to add them as multiversiin variants 
> > to the original function.
>
> Why wouldn't it be correct to pick the version through the overload 
> resolution instead of the code generation?
>  How this could work is already described in the TODO (CodeGenModule.cpp):
>
>   // TODO: We should introduce function aliases for `omp declare variant`
>   //   directives such that we can treat them through the same overload
>   //   resolution scheme (via multi versioning) as `omp begin declare
>   //   variant` functions. For an `omp declare variant(VARIANT) ...`
>   //   that is attached to a BASE function we would create a global alias
>   //   VARIANT = BASE which will participate in the multi version overload
>   //   resolution. If picked, here is no need to emit them explicitly.
>   
>
>
>
>  ---
>
> I still haven't understood why we cannot/should not reuse the existing 
> multi-version support and instead duplicate the logic in some custom scheme.
>  We have this patch that shows how we can reuse the logic in Clang. It works 
> on a per-call basis, so it will work for all context selector (incl. 
> construct).
>  If you think there is something conceptually not working, I'd like to hear 
> about it. However, just saying "it wouldn't be correct" is not sufficient. 
> You need to provide details about the situation, what you think would not 
> work, and why.


I explayned already: declare variant cannot be represented as mutiversion 
functiin, for example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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 wouldn't it be correct to pick the version through the overload resolution 
instead of the code generation?
How this could work is already described in the TODO (CodeGenModule.cpp):

  // TODO: We should introduce function aliases for `omp declare variant`
  //   directives such that we can treat them through the same overload
  //   resolution scheme (via multi versioning) as `omp begin declare
  //   variant` functions. For an `omp declare variant(VARIANT) ...`
  //   that is attached to a BASE function we would create a global alias
  //   VARIANT = BASE which will participate in the multi version overload
  //   resolution. If picked, here is no need to emit them explicitly.



---

I still haven't understood why we cannot/should not reuse the existing 
multi-version support and instead duplicate the logic in some custom scheme.
We have this patch that shows how we can reuse the logic in Clang. It works on 
a per-call basis, so it will work for all context selector (incl. construct).
If you think there is something conceptually not working, I'd like to hear 
about it. However, just saying "it wouldn't be correct" is not sufficient. You 
need to provide details about the situation, what you think would not work, and 
why.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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 check what abs declarations where here and which were not. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D71179#1776108 , @jdoerfert wrote:

> 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 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. Generally speaking, there 
> > > > are 2 kinds of traits - global traits (like `vendor`, `kind`, `isa`, 
> > > > etc.), which can be resolved completely statically and do not need 
> > > > multiversioning, and local traits, like `construct`, which depend on 
> > > > the OpenMP directives and require something similar to the 
> > > > multiversioning.
> > >
> > > The case where the code is skipped is easy, sure. However, if we "could 
> > > easily resolve" the other case, we could have implemented an #ifdef 
> > > solution for `math.h/cmath`. This was not the case and still is not. We 
> > > basically populate the namespace with multiple versions of the same 
> > > function (with the same name) and then select the appropriate one for 
> > > each call site.
> > >
> > > Instead of trying to argue why this is not needed for some cases, could 
> > > you argue why we should have multiple schemes to resolve all types of 
> > > variants? It seems you inherently assume the codegen patching scheme 
> > > implemented right now is useful even if we need something else to 
> > > complement it. I don't think so, thus there is little reason for me to 
> > > distinguish between the types of variants that need multi-version support 
> > > ant the types that can be implemented with multi-versions but don't need 
> > > it.
> >
> >
> > Because each particular problem requires its own solution and it is always 
> > a bad idea to use the microscope to hammer the nails.
>
>
> While I see where you are coming from, I disagree. We have a generic 
> framework available that we already need to use in some cases, there is no 
> harm in using it for all cases. It would be different if we wouldn't need the 
> generic framework at all, but that is not the case. All I ask is to literally 
> share existing code, no additional complexity needed. Your suggestion will 
> complicate the setup, duplicate logic, and make it overall harder to maintain 
> and compose in the future. If you still disagree, please provide some 
> arguments (and details) why we would benefit from your setup.


I have different opinion. You can reuse existing codegen for declare variant 
functions with global context selectors only. You just need to iterate through 
all the variants and choose the best one.
That's why you don't need the dispatching in your scheme. You're doing 
absolutely the same thing as the original declare variant implementation.

We cannot use multiversioning for the original declare variant construct since 
there is no multiversioning at all. We have a single function with many 
different aliasing functions, having different names. They are completely 
different functions. And I don't think it would be correct to add them as 
multiversiin variants to the original function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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 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. Generally speaking, there are 
> > > 2 kinds of traits - global traits (like `vendor`, `kind`, `isa`, etc.), 
> > > which can be resolved completely statically and do not need 
> > > multiversioning, and local traits, like `construct`, which depend on the 
> > > OpenMP directives and require something similar to the multiversioning.
> >
> > The case where the code is skipped is easy, sure. However, if we "could 
> > easily resolve" the other case, we could have implemented an #ifdef 
> > solution for `math.h/cmath`. This was not the case and still is not. We 
> > basically populate the namespace with multiple versions of the same 
> > function (with the same name) and then select the appropriate one for each 
> > call site.
> >
> > Instead of trying to argue why this is not needed for some cases, could you 
> > argue why we should have multiple schemes to resolve all types of variants? 
> > It seems you inherently assume the codegen patching scheme implemented 
> > right now is useful even if we need something else to complement it. I 
> > don't think so, thus there is little reason for me to distinguish between 
> > the types of variants that need multi-version support ant the types that 
> > can be implemented with multi-versions but don't need it.
>
>
> Because each particular problem requires its own solution and it is always a 
> bad idea to use the microscope to hammer the nails.


While I see where you are coming from, I disagree. We have a generic framework 
available that we already need to use in some cases, there is no harm in using 
it for all cases. It would be different if we wouldn't need the generic 
framework at all, but that is not the case. All I ask is to literally share 
existing code, no additional complexity needed. Your suggestion will complicate 
the setup, duplicate logic, and make it overall harder to maintain and compose 
in the future. If you still disagree, please provide some arguments (and 
details) why we would benefit from your setup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Alexey Bataev via Phabricator via cfe-commits]

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 `construct` because all other traits can 
> > be easily resolved at the compile time. Generally speaking, there are 2 
> > kinds of traits - global traits (like `vendor`, `kind`, `isa`, etc.), which 
> > can be resolved completely statically and do not need multiversioning, and 
> > local traits, like `construct`, which depend on the OpenMP directives and 
> > require something similar to the multiversioning.
>
> The case where the code is skipped is easy, sure. However, if we "could 
> easily resolve" the other case, we could have implemented an #ifdef solution 
> for `math.h/cmath`. This was not the case and still is not. We basically 
> populate the namespace with multiple versions of the same function (with the 
> same name) and then select the appropriate one for each call site.
>
> Instead of trying to argue why this is not needed for some cases, could you 
> argue why we should have multiple schemes to resolve all types of variants? 
> It seems you inherently assume the codegen patching scheme implemented right 
> now is useful even if we need something else to complement it. I don't think 
> so, thus there is little reason for me to distinguish between the types of 
> variants that need multi-version support ant the types that can be 
> implemented with multi-versions but don't need it.


Because each particular problem requires its own solution and it is always a 
bad idea to use the microscope to hammer the nails.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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. Generally speaking, there are 2 kinds of traits 
> - global traits (like `vendor`, `kind`, `isa`, etc.), which can be resolved 
> completely statically and do not need multiversioning, and local traits, like 
> `construct`, which depend on the OpenMP directives and require something 
> similar to the multiversioning.

The case where the code is skipped is easy, sure. However, if we "could easily 
resolve" the other case, we could have implemented an #ifdef solution for 
`math.h/cmath`. This was not the case and still is not. We basically populate 
the namespace with multiple versions of the same function (with the same name) 
and then select the appropriate one for each call site.

Instead of trying to argue why this is not needed for some cases, could you 
argue why we should have multiple schemes to resolve all types of variants? It 
seems you inherently assume the codegen patching scheme implemented right now 
is useful even if we need something else to complement it. I don't think so, 
thus there is little reason for me to distinguish between the types of variants 
that need multi-version support ant the types that can be implemented with 
multi-versions but don't need it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Alexey Bataev via Phabricator via cfe-commits]

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 example we for math) will inevitably use 
> >> begin/end declare variant.
> > 
> > I rather doubt that it is so much broken. The fact, that you need some new 
> > construct to express some functionality does not mean that the previous one 
> > is incorrect. It is incomplete, maybe. But not broken.
>
> Broken in the sense that we (in the OpenMP accelerator subcommittee) don't 
> think it can be used for what we envisioned it initially. It can be used for 
> certain things though.
>
> > And even for begin/end stuff, multiversioning is only required for 
> > construct traits, for all other traits we can reuse the existing 
> > implementation.
>
> Again, this is not the case. begin/end *always* caused multiple definitions 
> with the same name. Even if we ignore that for a second, why should we not 
> use the powerful infrastructure we have (=multi-versioning) that supports 
> `construct` traits and not use it for the other traits? Or asked differently, 
> why should we have a second codegen rewriting scheme?


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. Generally speaking, there are 2 kinds of traits - global 
traits (like `vendor`, `kind`, `isa`, etc.), which can be resolved completely 
statically and do not need multiversioning, and local traits, like `construct`, 
which depend on the OpenMP directives and require something similar to the 
multiversioning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

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: Clang.CXX/special/class_inhctor/p3.cpp
  failed: Clang.Headers/nvptx_device_cmath_functions.c
  failed: Clang.Headers/nvptx_device_cmath_functions.cpp
  failed: Clang.Headers/nvptx_device_cmath_functions_cxx17.cpp
  failed: Clang.Headers/nvptx_device_math_functions.c
  failed: Clang.Headers/nvptx_device_math_functions.cpp
  failed: Clang.Headers/nvptx_device_math_functions_cxx17.cpp
  failed: Clang.OpenMP/declare_variant_ast_print.cpp
  failed: Clang.OpenMP/declare_variant_device_kind_codegen.cpp
  failed: Clang.OpenMP/declare_variant_implementation_vendor_codegen.cpp
  failed: Clang.OpenMP/declare_variant_messages.c
  failed: Clang.OpenMP/declare_variant_messages.cpp
  failed: Clang.OpenMP/declare_variant_mixed_codegen.cpp
  failed: Clang.OpenMP/math_codegen.cpp
  failed: Clang.OpenMP/math_fp_macro.cpp
  failed: Clang.OpenMP/nvptx_declare_variant_device_kind_codegen.cpp
  failed: Clang.OpenMP/nvptx_declare_variant_implementation_vendor_codegen.cpp
  failed: Clang.SemaCXX/attr-cpuspecific.cpp
  failed: Clang.SemaCXX/attr-target-mv.cpp
  failed: Clang.SemaCXX/friend.cpp
  failed: Clang.SemaCXX/using-decl-1.cpp

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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 doubt that it is so much broken. The fact, that you need some new 
> construct to express some functionality does not mean that the previous one 
> is incorrect. It is incomplete, maybe. But not broken.

Broken in the sense that we (in the OpenMP accelerator subcommittee) don't 
think it can be used for what we envisioned it initially. It can be used for 
certain things though.

> And even for begin/end stuff, multiversioning is only required for construct 
> traits, for all other traits we can reuse the existing implementation.

Again, this is not the case. begin/end *always* caused multiple definitions 
with the same name. Even if we ignore that for a second, why should we not use 
the powerful infrastructure we have (=multi-versioning) that supports 
`construct` traits and not use it for the other traits? Or asked differently, 
why should we have a second codegen rewriting scheme?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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 copy the complex cuda_runtime_wrapper include magic.
However, the `sin(long double)` is back if it is called in a function that has
a target region.  This is an artifact unrelated to any of this (I would argue).
The problem is that we parse + type check *host* code surrounding the target
region when we compile for the target. This has various down sites and can
easily break without math involvement. Long story short, we need to fix this
later separately.


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/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Headers/__clang_cuda_cmath.h
  clang/lib/Headers/__clang_cuda_device_functions.h
  clang/lib/Headers/__clang_cuda_math_forward_declares.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  clang/lib/Headers/openmp_wrappers/cmath
  clang/lib/Headers/openmp_wrappers/math.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant.c
  clang/test/OpenMP/begin_declare_variant_codegen.cpp
  clang/test/OpenMP/math_codegen.cpp
  clang/test/OpenMP/math_fp_macro.cpp

Index: clang/test/OpenMP/math_fp_macro.cpp
===
--- /dev/null
+++ clang/test/OpenMP/math_fp_macro.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+#include 
+
+int main() {
+  double a(0);
+  return (std::fpclassify(a) != FP_ZERO);
+}
Index: clang/test/OpenMP/math_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/math_codegen.cpp
@@ -0,0 +1,15 @@
+#include 
+
+void math(short s, int i, float f, double d) {
+  sin(s);
+  sin(i);
+  sin(f);
+  sin(d);
+}
+
+void foo(short s, int i, float f, double d, long double ld) {
+  //sin(ld);
+  math(s, i, f, d);
+#pragma omp target
+  { math(s, i, f, d); }
+}
Index: clang/test/OpenMP/begin_declare_variant_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/begin_declare_variant_codegen.cpp
@@ -0,0 +1,134 @@
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+int bar(void) {
+  return 0;
+}
+
+template 
+T baz(void) { return 0; }
+
+#pragma omp begin declare variant match(device={kind(cpu)})
+int foo(void) {
+  return 1;
+}
+int bar(void) {
+  return 1;
+}
+template 
+T baz(void) { return 1; }
+
+template 
+T biz(void) { return 1; }
+
+template 
+T buz(void) { return 3; }
+
+template <>
+char buz(void) { return 1; }
+
+template 
+T bez(void) { return 3; }
+#pragma omp end declare variant
+
+#pragma omp begin declare variant match(device={kind(gpu)})
+int foo(void) {
+  return 2;
+}
+int bar(void) {
+  return 2;
+}
+#pragma omp end declare variant
+
+
+#pragma omp begin declare variant match(device={kind(fpga)})
+
+This text is never parsed!
+
+#pragma omp end declare variant
+
+int foo(void) {
+  return 0;
+}
+
+template 
+T biz(void) { return 0; }
+
+template <>
+char buz(void) { return 0; }
+
+template <>
+long bez(void) { return 0; }
+
+#pragma omp begin declare variant match(device = {kind(cpu)})
+template <>
+long bez(void) { return 1; }
+#pragma omp end declare variant
+
+int test() {
+  return foo() + bar() + baz() + biz() + buz() + bez();
+}
+
+// Make sure all ompvariant functions return 1 and all others return 0.
+
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3barv()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 0
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3foov.ompvariant()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 1
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3barv.ompvariant()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 1
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define signext i8 

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

[Johannes Doerfert via Phabricator via cfe-commits]

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 place. It's been available in this header for a 
> long time and it *is* needed.
> 
> 
> 
Done.



Comment at: clang/lib/Headers/__clang_cuda_cmath.h:462
-#ifndef _OPENMP
-using ::scalblnf;
-#endif

tra wrote:
> I think only `#ifdef` should be removed here. `scalblnf` itself should remain.
I misinterpreted the TODOs, here and above. That is why I removed code. Sorry 
for the noise.



Comment at: clang/lib/Headers/__clang_cuda_device_functions.h:1724
-#ifndef _OPENMP
-__DEVICE__ double scalbln(double __a, long __b) {
-  if (__b > INT_MAX)

tra wrote:
> Ditto here. Only preprocessor statements should be removed.
Yeah, my bad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Alexey Bataev via Phabricator via cfe-commits]

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 regarding math.h? I recall we still 
> > had problems with host code needing the long-double overloads, with 
> > constants from the system headers, etc.
>
> The three tests I have in here show already that almost all of the known 
> problems are solved by this (e.g. constants from the system headers). The 
> rest can be easily added as lit test. The test suite situation is evolving 
> but far from me being resolved. I would prefer not to mix these discussions 
> and focus on lit tests with this patch (once split).
>
> >> 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 it will solve all our math.h problems already).
> >>  However, we need to avoid collisions with user code, e.g., through the 
> >> use of symbols in the name that are not allowed to be used by the user (I 
> >> thought "." is one of them).
> > 
> > Okay, but how to we distinguish functions for which there is a declaration 
> > and we need the mangling because the user has provided a definition 
> > elsewhere, from those for which there is a declaration, and we don't want 
> > mangling because we need to link to some system library?
>
> The idea is, declarations inside begin/end declare variant are supposed to be 
> not affected by the begin/end declare variant. That is, if you have 
> declarations you cannot expect variant multi-versioning to happen. Having 
> declarations inside or outside the begin/end declare variant is still fine if 
> they all denote the same function.
>
> > I don't think we should do this. Something similar to multiversioning is 
> > required only for a small subset.
>
> 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 doubt that it is so much broken. The fact, that you need some new 
construct to express some functionality does not mean that the previous one is 
incorrect. It is incomplete, maybe. But not broken. And even for begin/end 
stuff, multiversioning is only required for construct traits, for all other 
traits we can reuse the existing implementation.

> 
> 
>> Everything else can be implemented in a more straightforward and simple way.
> 
> Having a single scheme is arguably simpler than maintaining multiple schemes. 
> There is no additional overhead in using the more powerful and available 
> multi-version scheme for everything.
> 
>>   Plus, I'm not sure that we'll need full reuse of the multiversioning. 
>> Seems to me, we can implement codegen in a different way. 
> 
> Please provide actual details with statements like this. It is impossible to 
> tell what you mean.
> 
>> Multiversioning is supported only by x86 in clang/LLVM. I think we can try 
>> to implement a more portable and universal scheme.
> 
> This is not true, at least not from a conceptual standpoint. While 
> cpu_supports and cpu_is multi-versioning is restricted to X86, see 
> `supportsMultiVersioning` in TargetInfo.h,  the new kind of OpenMP 
> multi-versioning is a portable and universal scheme (see the uses of 
> `supportsMultiVersioning`)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

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: Clang.CXX/special/class_inhctor/p3.cpp
  failed: Clang.Headers/nvptx_device_cmath_functions.c
  failed: Clang.Headers/nvptx_device_math_functions.c
  failed: Clang.OpenMP/declare_variant_ast_print.cpp
  failed: Clang.OpenMP/declare_variant_device_kind_codegen.cpp
  failed: Clang.OpenMP/declare_variant_implementation_vendor_codegen.cpp
  failed: Clang.OpenMP/declare_variant_messages.c
  failed: Clang.OpenMP/declare_variant_messages.cpp
  failed: Clang.OpenMP/declare_variant_mixed_codegen.cpp
  failed: Clang.OpenMP/math_codegen.cpp
  failed: Clang.OpenMP/math_fp_macro.cpp
  failed: Clang.OpenMP/nvptx_declare_variant_device_kind_codegen.cpp
  failed: Clang.OpenMP/nvptx_declare_variant_implementation_vendor_codegen.cpp
  failed: Clang.SemaCXX/attr-cpuspecific.cpp
  failed: Clang.SemaCXX/attr-target-mv.cpp
  failed: Clang.SemaCXX/friend.cpp
  failed: Clang.SemaCXX/using-decl-1.cpp

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Headers/__clang_cuda_cmath.h
  clang/lib/Headers/__clang_cuda_device_functions.h
  clang/lib/Headers/__clang_cuda_math_forward_declares.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  clang/lib/Headers/openmp_wrappers/cmath
  clang/lib/Headers/openmp_wrappers/math.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant.c
  clang/test/OpenMP/begin_declare_variant_codegen.cpp
  clang/test/OpenMP/math_codegen.cpp
  clang/test/OpenMP/math_fp_macro.cpp

Index: clang/test/OpenMP/math_fp_macro.cpp
===
--- /dev/null
+++ clang/test/OpenMP/math_fp_macro.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+#include 
+
+int main() {
+  double a(0);
+  return (std::fpclassify(a) != FP_ZERO);
+}
Index: clang/test/OpenMP/math_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/math_codegen.cpp
@@ -0,0 +1,15 @@
+#include 
+
+void math(short s, int i, float f, double d) {
+  sin(s);
+  sin(i);
+  sin(f);
+  sin(d);
+}
+
+void foo(short s, int i, float f, double d, long double ld) {
+  sin(ld);
+  math(s, i, f, d);
+#pragma omp target
+  { math(s, i, f, d); }
+}
Index: clang/test/OpenMP/begin_declare_variant_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/begin_declare_variant_codegen.cpp
@@ -0,0 +1,134 @@
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+int bar(void) {
+  return 0;
+}
+
+template 
+T baz(void) { return 0; }
+
+#pragma omp begin declare variant match(device={kind(cpu)})
+int foo(void) {
+  return 1;
+}
+int bar(void) {
+  return 1;
+}
+template 
+T baz(void) { return 1; }
+
+template 
+T biz(void) { return 1; }
+
+template 
+T buz(void) { return 3; }
+
+template <>
+char buz(void) { return 1; }
+
+template 
+T bez(void) { return 3; }
+#pragma omp end declare variant
+
+#pragma omp begin declare variant match(device={kind(gpu)})
+int foo(void) {
+  return 2;
+}
+int bar(void) {
+  return 2;
+}
+#pragma omp end declare variant
+
+
+#pragma omp begin declare variant match(device={kind(fpga)})
+
+This text is never parsed!
+
+#pragma omp end declare variant
+
+int foo(void) {
+  return 0;
+}
+
+template 
+T biz(void) { return 0; }
+
+template <>
+char buz(void) { return 0; }
+
+template <>
+long bez(void) { return 0; }
+
+#pragma omp begin declare variant match(device = {kind(cpu)})
+template <>
+long bez(void) { return 1; }
+#pragma omp end declare variant
+
+int test() {
+  return foo() + bar() + baz() + biz() + buz() + bez();
+}
+
+// Make sure all ompvariant functions return 1 and all others return 0.
+
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3barv()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 0
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3foov.ompvariant()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 1
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3barv.ompvariant()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 1
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define signext i8 @_Z3buzIcET_v.ompvariant()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i8 1
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3foov()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 0
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define signext i8 @_Z3buzIcET_v()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i8 0
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i64 @_Z3bezIlET_v()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i64 0
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i64 @_Z3bezIlET_v.ompvariant()
+// CHECK-NEXT:  entry:
+// 

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

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: Clang.CXX/special/class_inhctor/p3.cpp
  failed: Clang.Headers/nvptx_device_cmath_functions.c
  failed: Clang.Headers/nvptx_device_cmath_functions.cpp
  failed: Clang.Headers/nvptx_device_cmath_functions_cxx17.cpp
  failed: Clang.Headers/nvptx_device_math_functions.c
  failed: Clang.Headers/nvptx_device_math_functions.cpp
  failed: Clang.Headers/nvptx_device_math_functions_cxx17.cpp
  failed: Clang.OpenMP/declare_variant_ast_print.cpp
  failed: Clang.OpenMP/declare_variant_device_kind_codegen.cpp
  failed: Clang.OpenMP/declare_variant_implementation_vendor_codegen.cpp
  failed: Clang.OpenMP/declare_variant_messages.c
  failed: Clang.OpenMP/declare_variant_messages.cpp
  failed: Clang.OpenMP/declare_variant_mixed_codegen.cpp
  failed: Clang.OpenMP/math_codegen.cpp
  failed: Clang.OpenMP/math_fp_macro.cpp
  failed: Clang.OpenMP/nvptx_declare_variant_device_kind_codegen.cpp
  failed: Clang.OpenMP/nvptx_declare_variant_implementation_vendor_codegen.cpp
  failed: Clang.SemaCXX/attr-cpuspecific.cpp
  failed: Clang.SemaCXX/attr-target-mv.cpp
  failed: Clang.SemaCXX/friend.cpp
  failed: Clang.SemaCXX/using-decl-1.cpp

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Artem Belevich via Phabricator via cfe-commits]

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 available in this header for a long 
time and it *is* needed.






Comment at: clang/lib/Headers/__clang_cuda_cmath.h:462
-#ifndef _OPENMP
-using ::scalblnf;
-#endif

I think only `#ifdef` should be removed here. `scalblnf` itself should remain.



Comment at: clang/lib/Headers/__clang_cuda_device_functions.h:1724
-#ifndef _OPENMP
-__DEVICE__ double scalbln(double __a, long __b) {
-  if (__b > INT_MAX)

Ditto here. Only preprocessor statements should be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Hal Finkel via Phabricator via cfe-commits]

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 it will solve all our math.h problems already).
>>>  However, we need to avoid collisions with user code, e.g., through the use 
>>> of symbols in the name that are not allowed to be used by the user (I 
>>> thought "." is one of them).
>> 
>> Okay, but how to we distinguish functions for which there is a declaration 
>> and we need the mangling because the user has provided a definition 
>> elsewhere, from those for which there is a declaration, and we don't want 
>> mangling because we need to link to some system library?
> 
> The idea is, declarations inside begin/end declare variant are supposed to be 
> not affected by the begin/end declare variant. That is, if you have 
> declarations you cannot expect variant multi-versioning to happen. Having 
> declarations inside or outside the begin/end declare variant is still fine if 
> they all denote the same function.

Thanks, now I understand. This seems like it will work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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/include/clang/AST/Decl.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Headers/__clang_cuda_cmath.h
  clang/lib/Headers/__clang_cuda_device_functions.h
  clang/lib/Headers/__clang_cuda_math_forward_declares.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  clang/lib/Headers/openmp_wrappers/cmath
  clang/lib/Headers/openmp_wrappers/math.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant.c
  clang/test/OpenMP/begin_declare_variant_codegen.cpp
  clang/test/OpenMP/math_codegen.cpp
  clang/test/OpenMP/math_fp_macro.cpp

Index: clang/test/OpenMP/math_fp_macro.cpp
===
--- /dev/null
+++ clang/test/OpenMP/math_fp_macro.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+#include 
+
+int main() {
+  double a(0);
+  return (std::fpclassify(a) != FP_ZERO);
+}
Index: clang/test/OpenMP/math_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/math_codegen.cpp
@@ -0,0 +1,15 @@
+#include 
+
+void math(short s, int i, float f, double d) {
+  sin(s);
+  sin(i);
+  sin(f);
+  sin(d);
+}
+
+void foo(short s, int i, float f, double d, long double ld) {
+  sin(ld);
+  math(s, i, f, d);
+#pragma omp target
+  { math(s, i, f, d); }
+}
Index: clang/test/OpenMP/begin_declare_variant_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/begin_declare_variant_codegen.cpp
@@ -0,0 +1,134 @@
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+int bar(void) {
+  return 0;
+}
+
+template 
+T baz(void) { return 0; }
+
+#pragma omp begin declare variant match(device={kind(cpu)})
+int foo(void) {
+  return 1;
+}
+int bar(void) {
+  return 1;
+}
+template 
+T baz(void) { return 1; }
+
+template 
+T biz(void) { return 1; }
+
+template 
+T buz(void) { return 3; }
+
+template <>
+char buz(void) { return 1; }
+
+template 
+T bez(void) { return 3; }
+#pragma omp end declare variant
+
+#pragma omp begin declare variant match(device={kind(gpu)})
+int foo(void) {
+  return 2;
+}
+int bar(void) {
+  return 2;
+}
+#pragma omp end declare variant
+
+
+#pragma omp begin declare variant match(device={kind(fpga)})
+
+This text is never parsed!
+
+#pragma omp end declare variant
+
+int foo(void) {
+  return 0;
+}
+
+template 
+T biz(void) { return 0; }
+
+template <>
+char buz(void) { return 0; }
+
+template <>
+long bez(void) { return 0; }
+
+#pragma omp begin declare variant match(device = {kind(cpu)})
+template <>
+long bez(void) { return 1; }
+#pragma omp end declare variant
+
+int test() {
+  return foo() + bar() + baz() + biz() + buz() + bez();
+}
+
+// Make sure all ompvariant functions return 1 and all others return 0.
+
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3barv()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 0
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3foov.ompvariant()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 1
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3barv.ompvariant()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 1
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define signext i8 @_Z3buzIcET_v.ompvariant()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i8 1
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3foov()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 0
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define signext i8 @_Z3buzIcET_v()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i8 0
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i64 @_Z3bezIlET_v()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i64 0
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i64 

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

[Johannes Doerfert via Phabricator via cfe-commits]

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-double overloads, with constants 
> from the system headers, etc.

The three tests I have in here show already that almost all of the known 
problems are solved by this (e.g. constants from the system headers). The rest 
can be easily added as lit test. The test suite situation is evolving but far 
from me being resolved. I would prefer not to mix these discussions and focus 
on lit tests with this patch (once split).

>> 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 it will solve all our math.h problems already).
>>  However, we need to avoid collisions with user code, e.g., through the use 
>> of symbols in the name that are not allowed to be used by the user (I 
>> thought "." is one of them).
> 
> Okay, but how to we distinguish functions for which there is a declaration 
> and we need the mangling because the user has provided a definition 
> elsewhere, from those for which there is a declaration, and we don't want 
> mangling because we need to link to some system library?

The idea is, declarations inside begin/end declare variant are supposed to be 
not affected by the begin/end declare variant. That is, if you have 
declarations you cannot expect variant multi-versioning to happen. Having 
declarations inside or outside the begin/end declare variant is still fine if 
they all denote the same function.

> I don't think we should do this. Something similar to multiversioning is 
> required only for a small subset.

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.

> Everything else can be implemented in a more straightforward and simple way.

Having a single scheme is arguably simpler than maintaining multiple schemes. 
There is no additional overhead in using the more powerful and available 
multi-version scheme for everything.

>   Plus, I'm not sure that we'll need full reuse of the multiversioning. Seems 
> to me, we can implement codegen in a different way. 

Please provide actual details with statements like this. It is impossible to 
tell what you mean.

> Multiversioning is supported only by x86 in clang/LLVM. I think we can try to 
> implement a more portable and universal scheme.

This is not true, at least not from a conceptual standpoint. While cpu_supports 
and cpu_is multi-versioning is restricted to X86, see `supportsMultiVersioning` 
in TargetInfo.h,  the new kind of OpenMP multi-versioning is a portable and 
universal scheme (see the uses of `supportsMultiVersioning`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Hal Finkel via Phabricator via cfe-commits]

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 declare variant...", but, if we append this 
> > name to, for example, the names of functions present in the device math 
> > library, won't we have a problem with linking?
>
> 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 it will solve all our math.h problems already).
>  However, we need to avoid collisions with user code, e.g., through the use 
> of symbols in the name that are not allowed to be used by the user (I thought 
> "." is one of them).


Okay, but how to we distinguish functions for which there is a declaration and 
we need the mangling because the user has provided a definition elsewhere, from 
those for which there is a declaration, and we don't want mangling because we 
need to link to some system library?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Alexey Bataev via Phabricator via cfe-commits]

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 declare variant...", but, if we append this 
> > name to, for example, the names of functions present in the device math 
> > library, won't we have a problem with linking?
>
> 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 it will solve all our math.h problems already).
>  However, we need to avoid collisions with user code, e.g., through the use 
> of symbols in the name that are not allowed to be used by the user (I thought 
> "." is one of them).
>
> In D71179#1774639 , @JonChesterfield 
> wrote:
>
> > 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 generations from 
> > the same vendor.
> >
> > More ambitiously, one might want a GPU to be the host, and offload kernels 
> > for I/O to an aarch64 "target".
> >
> > We don't need to wire such combinations in up front, and I don't think 
> > they're excluded by this design. A future 'x86-64' variant would presumably 
> > be chosen over a 'cpu' variant when compiling for x86-64.
>
>
> As I wrote in the inline comment somewhere, `kind(gpu)` is an artifact due to 
> missing fine-grained context selectors. If that wasn't the core of your 
> issue, please elaborate.
>
> In D71179#1775157 , @ABataev wrote:
>
> > In D71179#1775066 , @hfinkel wrote:
> >
> > > In D71179#1774678 , @ABataev 
> > > wrote:
> > >
> > > > 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 several different functions 
> > > > > > with different names and for us no need to worry about overloading 
> > > > > > resolution, the compiler will do everything for us.
> > > > >
> > > > >
> > > > > I think we talk past each other again. This is the implementation of 
> > > > > `omp begin/end declare variant` as described in TR8. Bt definition, 
> > > > > the new variant mechanism will result in several different function 
> > > > > definitions with the same name. See the two tests for examples.
> > > >
> > > >
> > > > I just don't get it. If begin/end is just a something like 
> > > > #ifdef...endif, why you just can't skip everything between begin/end if 
> > > > the context does not match?
> > >
> > >
> > > The patch does this (see in ParseOpenMP.cpp where I asked about the 
> > > potential inf-loop). But when the definitions are not skipped, then we 
> > > have to worry about having multiple decls/defs of the same name and the 
> > > overload priorities.
> >
> >
> > I would recommend to drop all this extra stuff from the patch and focus on 
> > the initial patch. We'll need something similar to multiversion in case of 
> > the construct context selectors, but at first we need to solve all the 
> > problems with the simple versions of the construct rather that try to solve 
> > all the problems in the world in one patch. It is almost impossible to 
> > review.
>
>
> I agree with you to the point that this is not supposed to be reviewed. 
> That's why I wrote that in the commit message. I did this so we can make sure 
> the general path is clear and people (myself included) can see how/that it 
> works. 
>  I also agree that construct context selectors are very close to 
> multi-versioned functions. That is why I said earlier we should move all 
> variant handling into this scheme.


I don't think we should do this. Something similar to multiversioning is 
required only for a small subset. Everything else can be implemented in a more 
straightforward and simple way. Plus, I'm not sure that we'll need full reuse 
of the multiversioning. Seems to me, we can implement codegen in a different 
way. Multiversioning is supported only by x86 in clang/LLVM. I think we can try 
to implement a more portable and universal scheme.

> My plan:
> 
> - We play around with this prototype now, make sure there are no major 
> problems with it (so far it didn't seem so).
> - We split it up (This doesn't necessarily need to be only done by me, as 
> that often slows down these processes).
> - We review the parts with proper test coverage, etc. and 

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

[Johannes Doerfert via Phabricator via cfe-commits]

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 this name 
> to, for example, the names of functions present in the device math library, 
> won't we have a problem with linking?

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 it will solve all our math.h problems already).
However, we need to avoid collisions with user code, e.g., through the use of 
symbols in the name that are not allowed to be used by the user (I thought "." 
is one of them).

In D71179#1774639 , @JonChesterfield 
wrote:

> 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 generations from the same 
> vendor.
>
> More ambitiously, one might want a GPU to be the host, and offload kernels 
> for I/O to an aarch64 "target".
>
> We don't need to wire such combinations in up front, and I don't think 
> they're excluded by this design. A future 'x86-64' variant would presumably 
> be chosen over a 'cpu' variant when compiling for x86-64.


As I wrote in the inline comment somewhere, `kind(gpu)` is an artifact due to 
missing fine-grained context selectors. If that wasn't the core of your issue, 
please elaborate.

In D71179#1775157 , @ABataev wrote:

> In D71179#1775066 , @hfinkel wrote:
>
> > In D71179#1774678 , @ABataev wrote:
> >
> > > 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 several different functions with 
> > > > > different names and for us no need to worry about overloading 
> > > > > resolution, the compiler will do everything for us.
> > > >
> > > >
> > > > I think we talk past each other again. This is the implementation of 
> > > > `omp begin/end declare variant` as described in TR8. Bt definition, the 
> > > > new variant mechanism will result in several different function 
> > > > definitions with the same name. See the two tests for examples.
> > >
> > >
> > > I just don't get it. If begin/end is just a something like 
> > > #ifdef...endif, why you just can't skip everything between begin/end if 
> > > the context does not match?
> >
> >
> > The patch does this (see in ParseOpenMP.cpp where I asked about the 
> > potential inf-loop). But when the definitions are not skipped, then we have 
> > to worry about having multiple decls/defs of the same name and the overload 
> > priorities.
>
>
> I would recommend to drop all this extra stuff from the patch and focus on 
> the initial patch. We'll need something similar to multiversion in case of 
> the construct context selectors, but at first we need to solve all the 
> problems with the simple versions of the construct rather that try to solve 
> all the problems in the world in one patch. It is almost impossible to review.


I agree with you to the point that this is not supposed to be reviewed. That's 
why I wrote that in the commit message. I did this so we can make sure the 
general path is clear and people (myself included) can see how/that it works. 
I also agree that construct context selectors are very close to multi-versioned 
functions. That is why I said earlier we should move all variant handling into 
this scheme.

My plan:

- We play around with this prototype now, make sure there are no major problems 
with it (so far it didn't seem so).
- We split it up (This doesn't necessarily need to be only done by me, as that 
often slows down these processes).
- We review the parts with proper test coverage, etc. and get it in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D71179#1775157 , @ABataev wrote:

> In D71179#1775066 , @hfinkel wrote:
>
> > In D71179#1774678 , @ABataev wrote:
> >
> > > 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 several different functions with 
> > > > > different names and for us no need to worry about overloading 
> > > > > resolution, the compiler will do everything for us.
> > > >
> > > >
> > > > I think we talk past each other again. This is the implementation of 
> > > > `omp begin/end declare variant` as described in TR8. Bt definition, the 
> > > > new variant mechanism will result in several different function 
> > > > definitions with the same name. See the two tests for examples.
> > >
> > >
> > > I just don't get it. If begin/end is just a something like 
> > > #ifdef...endif, why you just can't skip everything between begin/end if 
> > > the context does not match?
> >
> >
> > The patch does this (see in ParseOpenMP.cpp where I asked about the 
> > potential inf-loop). But when the definitions are not skipped, then we have 
> > to worry about having multiple decls/defs of the same name and the overload 
> > priorities.
>
>
> I would recommend to drop all this extra stuff from the patch and focus on 
> the initial patch. We'll need something similar to multiversion in case of 
> the construct context selectors, but at first we need to solve all the 
> problems with the simple versions of the construct rather that try to solve 
> all the problems in the world in one patch. It is almost impossible to review.


I agree. We should split this into several patches (e.g., basic handling, 
skipping parsing for incompatible selectors, overload things). I think that 
@jdoerfert posted this so that people can see the high-level direction and 
provide feedback (including feedback on how to stage the functionality for 
review).

@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-double overloads, with constants from the 
system headers, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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 basically only moved, not written for this patch. It needs to 
> > life somewhere accessible from Parser to CodeGen, see the TODOs below.
> I don't think this is the right place for this code. Will try to move it to 
> Basic directory in my patch.
Sure. As noted in the TODOs, finding a place for this is needed.



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

JonChesterfield wrote:
> jdoerfert wrote:
> > As far as I can tell, `fpclassify` is not available in CUDA so it is 
> > unclear if we want to have it here or not. I removed it due to the TODO 
> > above. Consequently I also had to remove other `fpclassify` occurrences. If 
> > it turns out the host version is not usable on the device and we need the 
> > builtins, we add them back but under the opposite guard, that is `#ifdef 
> > _OPENMP`.
> We could call __builtin_fpclassify for nvptx, e.g. from 
> https://github.com/ROCm-Developer-Tools/aomp-extras/blob/0.7-6/aomp-device-libs/libm/src/libm-nvptx.cpp
> 
> ```int fpclassify(float __x) {
>   return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL, FP_SUBNORMAL, 
> FP_ZERO, __x);
> }
> int fpclassify(double __x) {
>   return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL, FP_SUBNORMAL, 
> FP_ZERO, __x);
> }
> ```
Agreed. Assuming it works, I'll put the fpclassify code back in but only remove 
the todo and OPENMP macro.



Comment at: clang/lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h:17
 
 #if defined(__NVPTX__) && defined(_OPENMP)
 

hfinkel wrote:
> Should we use a more-specific selector and then get rid of this `__NVPTX__` 
> check?
For now, this is CUDA after all. I was going to revisit this once we know how 
the AMD solution looks (I guess via HIP).
That said, I'd put a pin on it for now.

(The `kind(gpu)` selector below is only because we don't have anything more 
specific and it matches all our one GPU targets for now.)



Comment at: clang/lib/Parse/ParseOpenMP.cpp:1489
+++Nesting;
+} while (Nesting);
+

ABataev wrote:
> hfinkel wrote:
> > Will this just inf-loop if the file ends?
> It will.
We'll add a check and test.



Comment at: clang/test/OpenMP/begin_declare_variant_codegen.cpp:71
+}
+
+// Make sure all ompvariant functions return 1 and all others return 0.

JonChesterfield wrote:
> The name mangling should probably append the device kind, .e.g. 
> `_Z3foov.ompvariant.gpu`
There is already a TODO for that (I think CodeGenModule). Mangling right now is 
hardcoded and needs to be revisited :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D71179#1775066 , @hfinkel wrote:

> In D71179#1774678 , @ABataev wrote:
>
> > 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 several different functions with 
> > > > different names and for us no need to worry about overloading 
> > > > resolution, the compiler will do everything for us.
> > >
> > >
> > > I think we talk past each other again. This is the implementation of `omp 
> > > begin/end declare variant` as described in TR8. Bt definition, the new 
> > > variant mechanism will result in several different function definitions 
> > > with the same name. See the two tests for examples.
> >
> >
> > I just don't get it. If begin/end is just a something like #ifdef...endif, 
> > why you just can't skip everything between begin/end if the context does 
> > not match?
>
>
> The patch does this (see in ParseOpenMP.cpp where I asked about the potential 
> inf-loop). But when the definitions are not skipped, then we have to worry 
> about having multiple decls/defs of the same name and the overload priorities.


I would recommend to drop all this extra stuff from the patch and focus on the 
initial patch. We'll need something similar to multiversion in case of the 
construct context selectors, but at first we need to solve all the problems 
with the simple versions of the construct rather that try to solve all the 
problems in the world in one patch. It is almost impossible to review.




Comment at: clang/lib/AST/StmtOpenMP.cpp:2243
 }
+
+// TODO: We have various representations for the same data, it might help to

jdoerfert wrote:
> This code was basically only moved, not written for this patch. It needs to 
> life somewhere accessible from Parser to CodeGen, see the TODOs below.
I don't think this is the right place for this code. Will try to move it to 
Basic directory in my patch.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:1489
+++Nesting;
+} while (Nesting);
+

hfinkel wrote:
> Will this just inf-loop if the file ends?
It will.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D71179#1774678 , @ABataev wrote:

> 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 several different functions with different 
> > > names and for us no need to worry about overloading resolution, the 
> > > compiler will do everything for us.
> >
> >
> > I think we talk past each other again. This is the implementation of `omp 
> > begin/end declare variant` as described in TR8. Bt definition, the new 
> > variant mechanism will result in several different function definitions 
> > with the same name. See the two tests for examples.
>
>
> I just don't get it. If begin/end is just a something like #ifdef...endif, 
> why you just can't skip everything between begin/end if the context does not 
> match?


The patch does this (see in ParseOpenMP.cpp where I asked about the potential 
inf-loop). But when the definitions are not skipped, then we have to worry 
about having multiple decls/defs of the same name and the overload priorities.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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 tell, `fpclassify` is not available in CUDA so it is unclear 
> if we want to have it here or not. I removed it due to the TODO above. 
> Consequently I also had to remove other `fpclassify` occurrences. If it turns 
> out the host version is not usable on the device and we need the builtins, we 
> add them back but under the opposite guard, that is `#ifdef _OPENMP`.
We could call __builtin_fpclassify for nvptx, e.g. from 
https://github.com/ROCm-Developer-Tools/aomp-extras/blob/0.7-6/aomp-device-libs/libm/src/libm-nvptx.cpp

```int fpclassify(float __x) {
  return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL, FP_SUBNORMAL, 
FP_ZERO, __x);
}
int fpclassify(double __x) {
  return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL, FP_SUBNORMAL, 
FP_ZERO, __x);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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.ompvariant.gpu`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Alexey Bataev via Phabricator via cfe-commits]

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 several different functions with different names 
> > and for us no need to worry about overloading resolution, the compiler will 
> > do everything for us.
>
>
> I think we talk past each other again. This is the implementation of `omp 
> begin/end declare variant` as described in TR8. Bt definition, the new 
> variant mechanism will result in several different function definitions with 
> the same name. See the two tests for examples.


I just don't get it. If begin/end is just a something like #ifdef...endif, why 
you just can't skip everything between begin/end if the context does not match?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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 generations from the same 
vendor.

More ambitiously, one might want a GPU to be the host, and offload kernels for 
I/O to an aarch64 "target".

We don't need to wire such combinations in up front, and I don't think they're 
excluded by this design. A future 'x86-64' variant would presumably be chosen 
over a 'cpu' variant when compiling for x86-64.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Hal Finkel via Phabricator via cfe-commits]

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 several different functions with different names 
> > and for us no need to worry about overloading resolution, the compiler will 
> > do everything for us.
>
>
> I think we talk past each other again. This is the implementation of `omp 
> begin/end declare variant` as described in TR8. Bt definition, the new 
> variant mechanism will result in several different function definitions with 
> the same name. See the two tests for examples.


The intent of this feature is to allow us to include the device-function 
headers and the system headers simultaneously, giving preference to the device 
functions when compiling for the device, thus fixing a number of outstanding 
math.h OpenMP offloading problems. This definitely means that we'll have 
multiple functions with the same name and we need to pick the right ones during 
overload resolution.

@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 this name to, 
for example, the names of functions present in the device math library, won't 
we have a problem with linking?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Hal Finkel via Phabricator via cfe-commits]

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?



Comment at: clang/lib/Parse/ParseOpenMP.cpp:1489
+++Nesting;
+} while (Nesting);
+

Will this just inf-loop if the file ends?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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 overloading resolution, the compiler will 
> do everything for us.


I think we talk past each other again. This is the implementation of `omp 
begin/end declare variant` as described in TR8. Bt definition, the new variant 
mechanism will result in several different function definitions with the same 
name. See the two tests for examples.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D71179#1774470 , @jdoerfert wrote:

> In D71179#1774469 , @ABataev wrote:
>
> > 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 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 the 
> > > > checkscan be safely performed at the codegen phase.
> > >
> > >
> > > For better or worse we need this and it is actually a natural reuse of 
> > > the multi-versioning code. We need this because:
> > >
> > > 1. For the begin/end version we cannot even parse anything in a context 
> > > that does not match at encounter time, e.g. the `kind(fpga)` context in 
> > > `clang/test/AST/ast-dump-openmp-begin-declare-variant.c`.
> >
> >
> > Ok, here we can check the context and just skip everything between 
> > begin/end pragmas just like if something like #ifdef...#endif is seen.
>
>
> Agreed.
>
> >> 2. For the 5.0 version we cannot use the `replaceAllUses` approach 
> >> currently implemented in `tryEmitDeclareVariant` as soon as we have the 
> >> construct context selector trait. That means we will have to resolve the 
> >> call target earlier anyway.
> > 
> > I thought about this. Here we need to use a little bit different method, 
> > but again everything can be reolved at the codegen phase, no need to 
> > resolve it at parsing/sema.
>
> I doubt we can, yet alone want to do (basically) overload resolution during 
> codegen.
>  Depending on what function we resolve to, we get different instantiations 
> which require everything from semantic analysis to run on that code again, 
> right?
>  Could you elaborate why we should not do all the overload resolution at the 
> same time and with the same mechanism that is already present? I mean, 
> SemaOverload deals with multi-versioning already.


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 overloading resolution, the compiler will do 
everything for us.

> 
> 
>> Plus, this is completely different problem and must be solved in the 
>> different patch.
> 
> As I noted in the very beginning of the commit message, this is not supposed 
> to be a commited like this but split into multiple patches. Let's not mix 
> discussions here.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D71179#1774469 , @ABataev wrote:

> 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 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 the checkscan 
> > > be safely performed at the codegen phase.
> >
> >
> > For better or worse we need this and it is actually a natural reuse of the 
> > multi-versioning code. We need this because:
> >
> > 1. For the begin/end version we cannot even parse anything in a context 
> > that does not match at encounter time, e.g. the `kind(fpga)` context in 
> > `clang/test/AST/ast-dump-openmp-begin-declare-variant.c`.
>
>
> Ok, here we can check the context and just skip everything between begin/end 
> pragmas just like if something like #ifdef...#endif is seen.


Agreed.

>> 2. For the 5.0 version we cannot use the `replaceAllUses` approach currently 
>> implemented in `tryEmitDeclareVariant` as soon as we have the construct 
>> context selector trait. That means we will have to resolve the call target 
>> earlier anyway.
> 
> I thought about this. Here we need to use a little bit different method, but 
> again everything can be reolved at the codegen phase, no need to resolve it 
> at parsing/sema.

I doubt we can, yet alone want to do (basically) overload resolution during 
codegen.
Depending on what function we resolve to, we get different instantiations which 
require everything from semantic analysis to run on that code again, right?
Could you elaborate why we should not do all the overload resolution at the 
same time and with the same mechanism that is already present? I mean, 
SemaOverload deals with multi-versioning already.

> Plus, this is completely different problem and must be solved in the 
> different patch.

As I noted in the very beginning of the commit message, this is not supposed to 
be a commited like this but split into multiple patches. Let's not mix 
discussions here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Alexey Bataev via Phabricator via cfe-commits]

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 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 the checkscan be safely 
> > performed at the codegen phase.
>
>
> For better or worse we need this and it is actually a natural reuse of the 
> multi-versioning code. We need this because:
>
> 1. For the begin/end version we cannot even parse anything in a context that 
> does not match at encounter time, e.g. the `kind(fpga)` context in 
> `clang/test/AST/ast-dump-openmp-begin-declare-variant.c`.


Ok, here we can check the context and just skip everything between begin/end 
pragmas just like if something like #ifdef...#endif is seen.

> 2. For the 5.0 version we cannot use the `replaceAllUses` approach currently 
> implemented in `tryEmitDeclareVariant` as soon as we have the construct 
> context selector trait. That means we will have to resolve the call target 
> earlier anyway.

I thought about this. Here we need to use a little bit different method, but 
again everything can be reolved at the codegen phase, no need to resolve it at 
parsing/sema. Plus, this is completely different problem and must be solved in 
the different patch.

> (FWIW, I wrote this part of the SPEC.)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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 even need to move context checksnfrom 
> codegen, because with the current semantics all the checkscan be safely 
> performed at the codegen phase.


For better or worse we need this and it is actually a natural reuse of the 
multi-versioning code. We need this because:

1. For the begin/end version we cannot even parse anything in a context that 
does not match at encounter time, e.g. the `kind(fpga)` context in 
`clang/test/AST/ast-dump-openmp-begin-declare-variant.c`.
2. For the 5.0 version we cannot use the `replaceAllUses` approach currently 
implemented in `tryEmitDeclareVariant` as soon as we have the construct context 
selector trait. That means we will have to resolve the call target earlier 
anyway.

(FWIW, I wrote this part of the SPEC.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Alexey Bataev via Phabricator via cfe-commits]

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 the checkscan be safely performed at the 
codegen phase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: FAILURE - 
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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. It needs to 
life somewhere accessible from Parser to CodeGen, see the TODOs below.



Comment at: clang/lib/Headers/__clang_cuda_cmath.h:38
 #endif
 
-// For C++ 17 we need to include noexcept attribute to be compatible

NOTE: It might be cleaner to revert the patches that put the OpenMP handling 
code here first.



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

As far as I can tell, `fpclassify` is not available in CUDA so it is unclear if 
we want to have it here or not. I removed it due to the TODO above. 
Consequently I also had to remove other `fpclassify` occurrences. If it turns 
out the host version is not usable on the device and we need the builtins, we 
add them back but under the opposite guard, that is `#ifdef _OPENMP`.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:1048
 }
 
-/// Parse clauses for '#pragma omp declare variant ( variant-func-id ) clause'.

The diff is confusing here. I actually extracted some code into a helper 
function (`ParseOMPDeclareVariantMatchClause` on the right) which I can reuse 
in the begin/end handling. The code "deleted" here is below 
`ParseOMPDeclareVariantMatchClause` on the right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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/Decl.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Headers/__clang_cuda_cmath.h
  clang/lib/Headers/__clang_cuda_device_functions.h
  clang/lib/Headers/__clang_cuda_math_forward_declares.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  clang/lib/Headers/openmp_wrappers/cmath
  clang/lib/Headers/openmp_wrappers/math.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant.c
  clang/test/OpenMP/begin_declare_variant_codegen.cpp
  clang/test/OpenMP/math_fp_macro.cpp

Index: clang/test/OpenMP/math_fp_macro.cpp
===
--- /dev/null
+++ clang/test/OpenMP/math_fp_macro.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+#include 
+
+int main() {
+  double a(0);
+  return (std::fpclassify(a) != FP_ZERO);
+}
Index: clang/test/OpenMP/begin_declare_variant_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/begin_declare_variant_codegen.cpp
@@ -0,0 +1,134 @@
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+int bar(void) {
+  return 0;
+}
+
+template 
+T baz(void) { return 0; }
+
+#pragma omp begin declare variant match(device={kind(cpu)})
+int foo(void) {
+  return 1;
+}
+int bar(void) {
+  return 1;
+}
+template 
+T baz(void) { return 1; }
+
+template 
+T biz(void) { return 1; }
+
+template 
+T buz(void) { return 3; }
+
+template <>
+char buz(void) { return 1; }
+
+template 
+T bez(void) { return 3; }
+#pragma omp end declare variant
+
+#pragma omp begin declare variant match(device={kind(gpu)})
+int foo(void) {
+  return 2;
+}
+int bar(void) {
+  return 2;
+}
+#pragma omp end declare variant
+
+
+#pragma omp begin declare variant match(device={kind(fpga)})
+
+This text is never parsed!
+
+#pragma omp end declare variant
+
+int foo(void) {
+  return 0;
+}
+
+template 
+T biz(void) { return 0; }
+
+template <>
+char buz(void) { return 0; }
+
+template <>
+long bez(void) { return 0; }
+
+#pragma omp begin declare variant match(device = {kind(cpu)})
+template <>
+long bez(void) { return 1; }
+#pragma omp end declare variant
+
+int test() {
+  return foo() + bar() + baz() + biz() + buz() + bez();
+}
+
+// Make sure all ompvariant functions return 1 and all others return 0.
+
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3barv()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 0
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3foov.ompvariant()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 1
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3barv.ompvariant()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 1
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define signext i8 @_Z3buzIcET_v.ompvariant()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i8 1
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3foov()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 0
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define signext i8 @_Z3buzIcET_v()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i8 0
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i64 @_Z3bezIlET_v()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i64 0
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i64 @_Z3bezIlET_v.ompvariant()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i64 1
+// CHECK-NEXT:  }
+
+// Make sure we call only ompvariant functions
+
+// CHECK:  define i32 @_Z4testv()
+// CHECK:%call = call i32 @_Z3foov.ompvariant()
+// CHECK:%call1 = call i32 @_Z3barv.ompvariant()
+// CHECK:%call2 = call i32 @_Z3bazIiET_v.ompvariant()
+// CHECK:%call4 = call signext i16 @_Z3bizIsET_v.ompvariant()
+// CHECK:%call6 = call signext i8 @_Z3buzIcET_v.ompvariant()
+// 

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: FAILURE - 
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179



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

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

[Johannes Doerfert via Phabricator via cfe-commits]

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: clang.

NOTE: This is a WIP patch to foster a discussion. Please do consider
  that when browsing the code. Details will be discussed in
  individual commits once we agreed on the overall model. This is
  also the reason why test coverage, documentation, TODOs, etc. is
  lacking.

This patch provides initial support for `omp begin/end declare variant`,
as defined in OpenMP technical report 8 (TR8).

A major purpose of this patch is to provide proper math.h/cmath support
for OpenMP target offloading. See PR42061, PR42798, PR42799.
The three tests included in this patch show that these bugs (should be)
fixed in this scheme.

In contrast to the declare variant handling we already have, this patch
makes use of the multi-version handling in clang. This is especially
useful as the variants have the same name as the base functions. We
should try to port all OpenMP variant handling to this scheme, see the
TODO in CodeGenModule for a proposed way towards this goal. Other than
that, we tried to reuse the existing multi-version and OpenMP variant
handling as much as possible.

NOTE: There are various TODOs that need fixing and switches that need
  additional cases.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71179

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Headers/__clang_cuda_cmath.h
  clang/lib/Headers/__clang_cuda_device_functions.h
  clang/lib/Headers/__clang_cuda_math_forward_declares.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math.h
  clang/lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h
  clang/lib/Headers/openmp_wrappers/cmath
  clang/lib/Headers/openmp_wrappers/math.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant.c
  clang/test/OpenMP/begin_declare_variant_codegen.cpp
  clang/test/OpenMP/math_fp_macro.cpp

Index: clang/test/OpenMP/math_fp_macro.cpp
===
--- /dev/null
+++ clang/test/OpenMP/math_fp_macro.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+#include 
+
+int main() {
+  double a(0);
+  return (std::fpclassify(a) != FP_ZERO);
+}
Index: clang/test/OpenMP/begin_declare_variant_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/begin_declare_variant_codegen.cpp
@@ -0,0 +1,134 @@
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -emit-llvm %s -triple %itanium_abi_triple -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// expected-no-diagnostics
+
+int bar(void) {
+  return 0;
+}
+
+template 
+T baz(void) { return 0; }
+
+#pragma omp begin declare variant match(device={kind(cpu)})
+int foo(void) {
+  return 1;
+}
+int bar(void) {
+  return 1;
+}
+template 
+T baz(void) { return 1; }
+
+template 
+T biz(void) { return 1; }
+
+template 
+T buz(void) { return 3; }
+
+template <>
+char buz(void) { return 1; }
+
+template 
+T bez(void) { return 3; }
+#pragma omp end declare variant
+
+#pragma omp begin declare variant match(device={kind(gpu)})
+int foo(void) {
+  return 2;
+}
+int bar(void) {
+  return 2;
+}
+#pragma omp end declare variant
+
+
+#pragma omp begin declare variant match(device={kind(fpga)})
+
+This text is never parsed!
+
+#pragma omp end declare variant
+
+int foo(void) {
+  return 0;
+}
+
+template 
+T biz(void) { return 0; }
+
+template <>
+char buz(void) { return 0; }
+
+template <>
+long bez(void) { return 0; }
+
+#pragma omp begin declare variant match(device = {kind(cpu)})
+template <>
+long bez(void) { return 1; }
+#pragma omp end declare variant
+
+int test() {
+  return foo() + bar() + baz() + biz() + buz() + bez();
+}
+
+// Make sure all ompvariant functions return 1 and all others return 0.
+
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3barv()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 0
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs:
+// CHECK-NEXT:  define i32 @_Z3foov.ompvariant()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret i32 1
+// CHECK-NEXT:  }
+// CHECK:   ; Function Attrs: