[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
This revision was automatically updated to reflect the committed changes.
Hahnfeld marked an inline comment as done.
Closed by commit rL318601: [OpenMP] Show error if VLAs are not supported
(authored by Hahnfeld).
Changed prior to commit:
https://reviews.llvm.org/D39505?vs=123364=123473#toc
Repository:
rL LLVM
https://reviews.llvm.org/D39505
Files:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Basic/TargetInfo.h
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Basic/TargetInfo.cpp
cfe/trunk/lib/Basic/Targets/NVPTX.cpp
cfe/trunk/lib/Basic/Targets/SPIR.h
cfe/trunk/lib/Sema/SemaOpenMP.cpp
cfe/trunk/lib/Sema/SemaType.cpp
cfe/trunk/test/OpenMP/target_vla_messages.cpp
Index: cfe/trunk/include/clang/Sema/Sema.h
===
--- cfe/trunk/include/clang/Sema/Sema.h
+++ cfe/trunk/include/clang/Sema/Sema.h
@@ -8653,10 +8653,18 @@
NamedDeclSetType );
/// Check declaration inside target region.
void checkDeclIsAllowedInOpenMPTarget(Expr *E, Decl *D);
- /// Return true inside OpenMP target region.
+ /// Return true inside OpenMP declare target region.
bool isInOpenMPDeclareTargetContext() const {
return IsInOpenMPDeclareTargetContext;
}
+ /// Return true inside OpenMP target region.
+ bool isInOpenMPTargetExecutionDirective() const;
+ /// Return true if (un)supported features for the current target should be
+ /// diagnosed if OpenMP (offloading) is enabled.
+ bool shouldDiagnoseTargetSupportFromOpenMP() const {
+return !getLangOpts().OpenMPIsDevice || isInOpenMPDeclareTargetContext() ||
+ isInOpenMPTargetExecutionDirective();
+ }
/// Return the number of captured regions created for an OpenMP directive.
static int getOpenMPCaptureLevels(OpenMPDirectiveKind Kind);
Index: cfe/trunk/include/clang/Basic/TargetInfo.h
===
--- cfe/trunk/include/clang/Basic/TargetInfo.h
+++ cfe/trunk/include/clang/Basic/TargetInfo.h
@@ -60,6 +60,7 @@
// values are specified by the TargetInfo constructor.
bool BigEndian;
bool TLSSupported;
+ bool VLASupported;
bool NoAsmVariants; // True if {|} are normal characters.
bool HasFloat128;
unsigned char PointerWidth, PointerAlign;
@@ -939,6 +940,9 @@
return MaxTLSAlign;
}
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
/// \brief Whether the target supports SEH __try.
bool isSEHTrySupported() const {
return getTriple().isOSWindows() &&
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -141,6 +141,10 @@
"variable length array declaration cannot have 'extern' linkage">;
def ext_vla_folded_to_constant : Extension<
"variable length array folded to constant array as an extension">, InGroup;
+def err_vla_unsupported : Error<
+ "variable length arrays are not supported for the current target">;
+def note_vla_unsupported : Note<
+ "variable length arrays are not supported for the current target">;
// C99 variably modified types
def err_variably_modified_template_arg : Error<
@@ -8985,6 +8989,8 @@
"expected addressable reduction item for the task-based directives">;
def err_omp_reduction_with_nogroup : Error<
"'reduction' clause cannot be used with 'nogroup' clause">;
+def err_omp_reduction_vla_unsupported : Error<
+ "cannot generate code for reduction on %select{|array section, which requires a }0variable length array">;
} // end of OpenMP category
let CategoryName = "Related Result Type Issue" in {
Index: cfe/trunk/test/OpenMP/target_vla_messages.cpp
===
--- cfe/trunk/test/OpenMP/target_vla_messages.cpp
+++ cfe/trunk/test/OpenMP/target_vla_messages.cpp
@@ -0,0 +1,201 @@
+// PowerPC supports VLAs.
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-unknown-unknown -emit-llvm-bc %s -o %t-ppc-host-ppc.bc
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-unknown-unknown -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host-ppc.bc -o %t-ppc-device.ll
+
+// Nvidia GPUs don't support VLAs.
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host-nvptx.bc
+// RUN: %clang_cc1 -verify -DNO_VLA -fopenmp -x c++ -triple nvptx64-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host-nvptx.bc -o %t-nvptx-device.ll
+
+#ifndef NO_VLA
+// expected-no-diagnostics
+#endif
+
+#pragma omp
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, seems fine. Thanks for putting up with my questions. https://reviews.llvm.org/D39505 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
Hahnfeld marked an inline comment as done.
Hahnfeld added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
rjmccall wrote:
> Hahnfeld wrote:
> > rjmccall wrote:
> > > ABataev wrote:
> > > > Hahnfeld wrote:
> > > > > rjmccall wrote:
> > > > > > Hahnfeld wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > Hahnfeld wrote:
> > > > > > > > > rjmccall wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > > > > > > The way you've written this makes it sound
> > > > > > > > > > > > > > > > > > like "does the target support VLAs?", but
> > > > > > > > > > > > > > > > > > the actual semantic checks treat it as "do
> > > > > > > > > > > > > > > > > > OpenMP devices on this target support
> > > > > > > > > > > > > > > > > > VLAs?" Maybe there should be a more
> > > > > > > > > > > > > > > > > > specific way to query things about OpenMP
> > > > > > > > > > > > > > > > > > devices instead of setting a global flag
> > > > > > > > > > > > > > > > > > for the target?
> > > > > > > > > > > > > > > > > Actually, the NVPTX and SPIR targets never
> > > > > > > > > > > > > > > > > support VLAs. So I felt like it would be more
> > > > > > > > > > > > > > > > > correct to make this a global property of the
> > > > > > > > > > > > > > > > > target.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > The difference is that the other programming
> > > > > > > > > > > > > > > > > models (OpenCL and CUDA) error out
> > > > > > > > > > > > > > > > > immediatelyand regardless of the target
> > > > > > > > > > > > > > > > > because this limitation is reflected in the
> > > > > > > > > > > > > > > > > standards that disallow VLAs (see
> > > > > > > > > > > > > > > > > SemaType.cpp). For OpenMP we might have
> > > > > > > > > > > > > > > > > target devices that support VLA so we
> > > > > > > > > > > > > > > > > shouldn't error out for those.
> > > > > > > > > > > > > > > > If you want to make it a global property of the
> > > > > > > > > > > > > > > > target, that's fine, but then I don't
> > > > > > > > > > > > > > > > understand why your diagnostic only fires when
> > > > > > > > > > > > > > > > (S.isInOpenMPDeclareTargetContext() ||
> > > > > > > > > > > > > > > > S.isInOpenMPTargetExecutionDirective()).
> > > > > > > > > > > > > > > That is because of how OpenMP offloading works
> > > > > > > > > > > > > > > and how it is implemented in Clang. Consider the
> > > > > > > > > > > > > > > following snippet from the added test case:
> > > > > > > > > > > > > > > ```lang=c
> > > > > > > > > > > > > > > int vla[arg];
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > #pragma omp target map(vla[0:arg])
> > > > > > > > > > > > > > > {
> > > > > > > > > > > > > > >// more code here...
> > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Clang will take the following steps to compile
> > > > > > > > > > > > > > > this into a working binary for a GPU:
> > > > > > > > > > > > > > > 1. Parse and (semantically) analyze the code
> > > > > > > > > > > > > > > as-is for the host and produce LLVM Bitcode.
> > > > > > > > > > > > > > > 2. Parse and analyze again the code as-is and
> > > > > > > > > > > > > > > generate code for the offloading target, the GPU
> > > > > > > > > > > > > > > in this case.
> > > > > > > > > > > > > > > 3. Take LLVM Bitcode from 1., generate host
> > > > > > > > > > > > > > > binary and embed target binary from 3.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > `OpenMPIsDevice` will be true for 2., but the
> > > > > > > > > > > > > > > complete source code is analyzed. So to not throw
> > > > > > > > > > > > > > > errors for the host code, we have to make sure
> > > > > > > > > > > > > > > that we are actually generating code for the
> > > > > > > > > > > > > > > target device. This is either in a `target`
> > > > > > > > > > > > > > > directive or in a `declare target` region.
> > > > > > > > > > > > > > > Note that this is quite similar to what CUDA
> > > > > > > > > > > > > > > does, only they have `CUDADiagIfDeviceCode` for
> > > > > > > > > > > > > > > this logic. If you want me to add something of
> > > > > > > > > > > > > > > that kind for OpenMP target devices, I'm fine
> > > > > > > > > > > > > > > with that. However for the given case, it's a bit
> > > > > > > > > > > > > > > different because this error should only be
> > > > > > > > > > > > > > > thrown for target devices
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
Hahnfeld updated this revision to Diff 123364.
Hahnfeld added a comment.
Update changes to be generic.
https://reviews.llvm.org/D39505
Files:
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TargetInfo.h
include/clang/Sema/Sema.h
lib/Basic/TargetInfo.cpp
lib/Basic/Targets/NVPTX.cpp
lib/Basic/Targets/SPIR.h
lib/Sema/SemaOpenMP.cpp
lib/Sema/SemaType.cpp
test/OpenMP/target_vla_messages.cpp
Index: test/OpenMP/target_vla_messages.cpp
===
--- /dev/null
+++ test/OpenMP/target_vla_messages.cpp
@@ -0,0 +1,201 @@
+// PowerPC supports VLAs.
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-unknown-unknown -emit-llvm-bc %s -o %t-ppc-host-ppc.bc
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-unknown-unknown -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host-ppc.bc -o %t-ppc-device.ll
+
+// Nvidia GPUs don't support VLAs.
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host-nvptx.bc
+// RUN: %clang_cc1 -verify -DNO_VLA -fopenmp -x c++ -triple nvptx64-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host-nvptx.bc -o %t-nvptx-device.ll
+
+#ifndef NO_VLA
+// expected-no-diagnostics
+#endif
+
+#pragma omp declare target
+void declare(int arg) {
+ int a[2];
+#ifdef NO_VLA
+ // expected-error@+2 {{variable length arrays are not supported for the current target}}
+#endif
+ int vla[arg];
+}
+
+void declare_parallel_reduction(int arg) {
+ int a[2];
+
+#pragma omp parallel reduction(+: a)
+ { }
+
+#pragma omp parallel reduction(+: a[0:2])
+ { }
+
+#ifdef NO_VLA
+ // expected-error@+3 {{cannot generate code for reduction on array section, which requires a variable length array}}
+ // expected-note@+2 {{variable length arrays are not supported for the current target}}
+#endif
+#pragma omp parallel reduction(+: a[0:arg])
+ { }
+}
+#pragma omp end declare target
+
+template
+void target_template(int arg) {
+#pragma omp target
+ {
+#ifdef NO_VLA
+// expected-error@+2 {{variable length arrays are not supported for the current target}}
+#endif
+T vla[arg];
+ }
+}
+
+void target(int arg) {
+#pragma omp target
+ {
+#ifdef NO_VLA
+// expected-error@+2 {{variable length arrays are not supported for the current target}}
+#endif
+int vla[arg];
+ }
+
+#pragma omp target
+ {
+#pragma omp parallel
+{
+#ifdef NO_VLA
+// expected-error@+2 {{variable length arrays are not supported for the current target}}
+#endif
+ int vla[arg];
+}
+ }
+
+ target_template(arg);
+}
+
+void teams_reduction(int arg) {
+ int a[2];
+ int vla[arg];
+
+#pragma omp target map(a)
+#pragma omp teams reduction(+: a)
+ { }
+
+#ifdef NO_VLA
+ // expected-error@+4 {{cannot generate code for reduction on variable length array}}
+ // expected-note@+3 {{variable length arrays are not supported for the current target}}
+#endif
+#pragma omp target map(vla)
+#pragma omp teams reduction(+: vla)
+ { }
+
+#pragma omp target map(a[0:2])
+#pragma omp teams reduction(+: a[0:2])
+ { }
+
+#pragma omp target map(vla[0:2])
+#pragma omp teams reduction(+: vla[0:2])
+ { }
+
+#ifdef NO_VLA
+ // expected-error@+4 {{cannot generate code for reduction on array section, which requires a variable length array}}
+ // expected-note@+3 {{variable length arrays are not supported for the current target}}
+#endif
+#pragma omp target map(a[0:arg])
+#pragma omp teams reduction(+: a[0:arg])
+ { }
+
+#ifdef NO_VLA
+ // expected-error@+4 {{cannot generate code for reduction on array section, which requires a variable length array}}
+ // expected-note@+3 {{variable length arrays are not supported for the current target}}
+#endif
+#pragma omp target map(vla[0:arg])
+#pragma omp teams reduction(+: vla[0:arg])
+ { }
+}
+
+void parallel_reduction(int arg) {
+ int a[2];
+ int vla[arg];
+
+#pragma omp target map(a)
+#pragma omp parallel reduction(+: a)
+ { }
+
+#ifdef NO_VLA
+ // expected-error@+4 {{cannot generate code for reduction on variable length array}}
+ // expected-note@+3 {{variable length arrays are not supported for the current target}}
+#endif
+#pragma omp target map(vla)
+#pragma omp parallel reduction(+: vla)
+ { }
+
+#pragma omp target map(a[0:2])
+#pragma omp parallel reduction(+: a[0:2])
+ { }
+
+#pragma omp target map(vla[0:2])
+#pragma omp parallel reduction(+: vla[0:2])
+ { }
+
+#ifdef NO_VLA
+ // expected-error@+4 {{cannot generate code for reduction on array section, which requires a variable length array}}
+ // expected-note@+3 {{variable length arrays are not supported for the current target}}
+#endif
+#pragma omp target map(a[0:arg])
+#pragma omp parallel reduction(+: a[0:arg])
+ { }
+
+#ifdef NO_VLA
+ //
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
rjmccall added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
Hahnfeld wrote:
> rjmccall wrote:
> > ABataev wrote:
> > > Hahnfeld wrote:
> > > > rjmccall wrote:
> > > > > Hahnfeld wrote:
> > > > > > rjmccall wrote:
> > > > > > > Hahnfeld wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > > > > > The way you've written this makes it sound
> > > > > > > > > > > > > > > > > like "does the target support VLAs?", but the
> > > > > > > > > > > > > > > > > actual semantic checks treat it as "do OpenMP
> > > > > > > > > > > > > > > > > devices on this target support VLAs?" Maybe
> > > > > > > > > > > > > > > > > there should be a more specific way to query
> > > > > > > > > > > > > > > > > things about OpenMP devices instead of
> > > > > > > > > > > > > > > > > setting a global flag for the target?
> > > > > > > > > > > > > > > > Actually, the NVPTX and SPIR targets never
> > > > > > > > > > > > > > > > support VLAs. So I felt like it would be more
> > > > > > > > > > > > > > > > correct to make this a global property of the
> > > > > > > > > > > > > > > > target.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > The difference is that the other programming
> > > > > > > > > > > > > > > > models (OpenCL and CUDA) error out
> > > > > > > > > > > > > > > > immediatelyand regardless of the target because
> > > > > > > > > > > > > > > > this limitation is reflected in the standards
> > > > > > > > > > > > > > > > that disallow VLAs (see SemaType.cpp). For
> > > > > > > > > > > > > > > > OpenMP we might have target devices that
> > > > > > > > > > > > > > > > support VLA so we shouldn't error out for those.
> > > > > > > > > > > > > > > If you want to make it a global property of the
> > > > > > > > > > > > > > > target, that's fine, but then I don't understand
> > > > > > > > > > > > > > > why your diagnostic only fires when
> > > > > > > > > > > > > > > (S.isInOpenMPDeclareTargetContext() ||
> > > > > > > > > > > > > > > S.isInOpenMPTargetExecutionDirective()).
> > > > > > > > > > > > > > That is because of how OpenMP offloading works and
> > > > > > > > > > > > > > how it is implemented in Clang. Consider the
> > > > > > > > > > > > > > following snippet from the added test case:
> > > > > > > > > > > > > > ```lang=c
> > > > > > > > > > > > > > int vla[arg];
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > #pragma omp target map(vla[0:arg])
> > > > > > > > > > > > > > {
> > > > > > > > > > > > > >// more code here...
> > > > > > > > > > > > > > }
> > > > > > > > > > > > > > ```
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Clang will take the following steps to compile this
> > > > > > > > > > > > > > into a working binary for a GPU:
> > > > > > > > > > > > > > 1. Parse and (semantically) analyze the code as-is
> > > > > > > > > > > > > > for the host and produce LLVM Bitcode.
> > > > > > > > > > > > > > 2. Parse and analyze again the code as-is and
> > > > > > > > > > > > > > generate code for the offloading target, the GPU in
> > > > > > > > > > > > > > this case.
> > > > > > > > > > > > > > 3. Take LLVM Bitcode from 1., generate host binary
> > > > > > > > > > > > > > and embed target binary from 3.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > `OpenMPIsDevice` will be true for 2., but the
> > > > > > > > > > > > > > complete source code is analyzed. So to not throw
> > > > > > > > > > > > > > errors for the host code, we have to make sure that
> > > > > > > > > > > > > > we are actually generating code for the target
> > > > > > > > > > > > > > device. This is either in a `target` directive or
> > > > > > > > > > > > > > in a `declare target` region.
> > > > > > > > > > > > > > Note that this is quite similar to what CUDA does,
> > > > > > > > > > > > > > only they have `CUDADiagIfDeviceCode` for this
> > > > > > > > > > > > > > logic. If you want me to add something of that kind
> > > > > > > > > > > > > > for OpenMP target devices, I'm fine with that.
> > > > > > > > > > > > > > However for the given case, it's a bit different
> > > > > > > > > > > > > > because this error should only be thrown for target
> > > > > > > > > > > > > > devices that don't support VLAs...
> > > > > > > > > > > > > I see. So the entire translation unit is re-parsed
> > > > > > > > > > > > > and re-Sema'ed from scratch for the target? Which
> > > > > > > > > > > > > means you need to avoid generating errors about
> > > > > > > > > > > >
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
Hahnfeld added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
rjmccall wrote:
> ABataev wrote:
> > Hahnfeld wrote:
> > > rjmccall wrote:
> > > > Hahnfeld wrote:
> > > > > rjmccall wrote:
> > > > > > Hahnfeld wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > > > > The way you've written this makes it sound like
> > > > > > > > > > > > > > > > "does the target support VLAs?", but the actual
> > > > > > > > > > > > > > > > semantic checks treat it as "do OpenMP devices
> > > > > > > > > > > > > > > > on this target support VLAs?" Maybe there
> > > > > > > > > > > > > > > > should be a more specific way to query things
> > > > > > > > > > > > > > > > about OpenMP devices instead of setting a
> > > > > > > > > > > > > > > > global flag for the target?
> > > > > > > > > > > > > > > Actually, the NVPTX and SPIR targets never
> > > > > > > > > > > > > > > support VLAs. So I felt like it would be more
> > > > > > > > > > > > > > > correct to make this a global property of the
> > > > > > > > > > > > > > > target.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > The difference is that the other programming
> > > > > > > > > > > > > > > models (OpenCL and CUDA) error out immediatelyand
> > > > > > > > > > > > > > > regardless of the target because this limitation
> > > > > > > > > > > > > > > is reflected in the standards that disallow VLAs
> > > > > > > > > > > > > > > (see SemaType.cpp). For OpenMP we might have
> > > > > > > > > > > > > > > target devices that support VLA so we shouldn't
> > > > > > > > > > > > > > > error out for those.
> > > > > > > > > > > > > > If you want to make it a global property of the
> > > > > > > > > > > > > > target, that's fine, but then I don't understand
> > > > > > > > > > > > > > why your diagnostic only fires when
> > > > > > > > > > > > > > (S.isInOpenMPDeclareTargetContext() ||
> > > > > > > > > > > > > > S.isInOpenMPTargetExecutionDirective()).
> > > > > > > > > > > > > That is because of how OpenMP offloading works and
> > > > > > > > > > > > > how it is implemented in Clang. Consider the
> > > > > > > > > > > > > following snippet from the added test case:
> > > > > > > > > > > > > ```lang=c
> > > > > > > > > > > > > int vla[arg];
> > > > > > > > > > > > >
> > > > > > > > > > > > > #pragma omp target map(vla[0:arg])
> > > > > > > > > > > > > {
> > > > > > > > > > > > >// more code here...
> > > > > > > > > > > > > }
> > > > > > > > > > > > > ```
> > > > > > > > > > > > >
> > > > > > > > > > > > > Clang will take the following steps to compile this
> > > > > > > > > > > > > into a working binary for a GPU:
> > > > > > > > > > > > > 1. Parse and (semantically) analyze the code as-is
> > > > > > > > > > > > > for the host and produce LLVM Bitcode.
> > > > > > > > > > > > > 2. Parse and analyze again the code as-is and
> > > > > > > > > > > > > generate code for the offloading target, the GPU in
> > > > > > > > > > > > > this case.
> > > > > > > > > > > > > 3. Take LLVM Bitcode from 1., generate host binary
> > > > > > > > > > > > > and embed target binary from 3.
> > > > > > > > > > > > >
> > > > > > > > > > > > > `OpenMPIsDevice` will be true for 2., but the
> > > > > > > > > > > > > complete source code is analyzed. So to not throw
> > > > > > > > > > > > > errors for the host code, we have to make sure that
> > > > > > > > > > > > > we are actually generating code for the target
> > > > > > > > > > > > > device. This is either in a `target` directive or in
> > > > > > > > > > > > > a `declare target` region.
> > > > > > > > > > > > > Note that this is quite similar to what CUDA does,
> > > > > > > > > > > > > only they have `CUDADiagIfDeviceCode` for this logic.
> > > > > > > > > > > > > If you want me to add something of that kind for
> > > > > > > > > > > > > OpenMP target devices, I'm fine with that. However
> > > > > > > > > > > > > for the given case, it's a bit different because this
> > > > > > > > > > > > > error should only be thrown for target devices that
> > > > > > > > > > > > > don't support VLAs...
> > > > > > > > > > > > I see. So the entire translation unit is re-parsed and
> > > > > > > > > > > > re-Sema'ed from scratch for the target? Which means
> > > > > > > > > > > > you need to avoid generating errors about things in the
> > > > > > > > > > > > outer translation unit that aren't part of the target
> > > > > > > > > > > > directive that you actually want to compile. I
> > > > > > > > > > > >
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
rjmccall added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
ABataev wrote:
> Hahnfeld wrote:
> > rjmccall wrote:
> > > Hahnfeld wrote:
> > > > rjmccall wrote:
> > > > > Hahnfeld wrote:
> > > > > > rjmccall wrote:
> > > > > > > ABataev wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > > > The way you've written this makes it sound like
> > > > > > > > > > > > > > > "does the target support VLAs?", but the actual
> > > > > > > > > > > > > > > semantic checks treat it as "do OpenMP devices on
> > > > > > > > > > > > > > > this target support VLAs?" Maybe there should be
> > > > > > > > > > > > > > > a more specific way to query things about OpenMP
> > > > > > > > > > > > > > > devices instead of setting a global flag for the
> > > > > > > > > > > > > > > target?
> > > > > > > > > > > > > > Actually, the NVPTX and SPIR targets never support
> > > > > > > > > > > > > > VLAs. So I felt like it would be more correct to
> > > > > > > > > > > > > > make this a global property of the target.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > The difference is that the other programming models
> > > > > > > > > > > > > > (OpenCL and CUDA) error out immediatelyand
> > > > > > > > > > > > > > regardless of the target because this limitation is
> > > > > > > > > > > > > > reflected in the standards that disallow VLAs (see
> > > > > > > > > > > > > > SemaType.cpp). For OpenMP we might have target
> > > > > > > > > > > > > > devices that support VLA so we shouldn't error out
> > > > > > > > > > > > > > for those.
> > > > > > > > > > > > > If you want to make it a global property of the
> > > > > > > > > > > > > target, that's fine, but then I don't understand why
> > > > > > > > > > > > > your diagnostic only fires when
> > > > > > > > > > > > > (S.isInOpenMPDeclareTargetContext() ||
> > > > > > > > > > > > > S.isInOpenMPTargetExecutionDirective()).
> > > > > > > > > > > > That is because of how OpenMP offloading works and how
> > > > > > > > > > > > it is implemented in Clang. Consider the following
> > > > > > > > > > > > snippet from the added test case:
> > > > > > > > > > > > ```lang=c
> > > > > > > > > > > > int vla[arg];
> > > > > > > > > > > >
> > > > > > > > > > > > #pragma omp target map(vla[0:arg])
> > > > > > > > > > > > {
> > > > > > > > > > > >// more code here...
> > > > > > > > > > > > }
> > > > > > > > > > > > ```
> > > > > > > > > > > >
> > > > > > > > > > > > Clang will take the following steps to compile this
> > > > > > > > > > > > into a working binary for a GPU:
> > > > > > > > > > > > 1. Parse and (semantically) analyze the code as-is for
> > > > > > > > > > > > the host and produce LLVM Bitcode.
> > > > > > > > > > > > 2. Parse and analyze again the code as-is and generate
> > > > > > > > > > > > code for the offloading target, the GPU in this case.
> > > > > > > > > > > > 3. Take LLVM Bitcode from 1., generate host binary and
> > > > > > > > > > > > embed target binary from 3.
> > > > > > > > > > > >
> > > > > > > > > > > > `OpenMPIsDevice` will be true for 2., but the complete
> > > > > > > > > > > > source code is analyzed. So to not throw errors for the
> > > > > > > > > > > > host code, we have to make sure that we are actually
> > > > > > > > > > > > generating code for the target device. This is either
> > > > > > > > > > > > in a `target` directive or in a `declare target` region.
> > > > > > > > > > > > Note that this is quite similar to what CUDA does, only
> > > > > > > > > > > > they have `CUDADiagIfDeviceCode` for this logic. If you
> > > > > > > > > > > > want me to add something of that kind for OpenMP target
> > > > > > > > > > > > devices, I'm fine with that. However for the given
> > > > > > > > > > > > case, it's a bit different because this error should
> > > > > > > > > > > > only be thrown for target devices that don't support
> > > > > > > > > > > > VLAs...
> > > > > > > > > > > I see. So the entire translation unit is re-parsed and
> > > > > > > > > > > re-Sema'ed from scratch for the target? Which means you
> > > > > > > > > > > need to avoid generating errors about things in the outer
> > > > > > > > > > > translation unit that aren't part of the target directive
> > > > > > > > > > > that you actually want to compile. I would've expected
> > > > > > > > > > > there to be some existing mechanism for that, to be
> > > > > > > > > > > honest, as opposed to explicitly trying to suppress
> > > > > > > > > > > target-specific diagnostics one by one.
> > > > > > > > > > Yes, that is my understanding.
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
ABataev added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
Hahnfeld wrote:
> rjmccall wrote:
> > Hahnfeld wrote:
> > > rjmccall wrote:
> > > > Hahnfeld wrote:
> > > > > rjmccall wrote:
> > > > > > ABataev wrote:
> > > > > > > ABataev wrote:
> > > > > > > > Hahnfeld wrote:
> > > > > > > > > rjmccall wrote:
> > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > > The way you've written this makes it sound like
> > > > > > > > > > > > > > "does the target support VLAs?", but the actual
> > > > > > > > > > > > > > semantic checks treat it as "do OpenMP devices on
> > > > > > > > > > > > > > this target support VLAs?" Maybe there should be a
> > > > > > > > > > > > > > more specific way to query things about OpenMP
> > > > > > > > > > > > > > devices instead of setting a global flag for the
> > > > > > > > > > > > > > target?
> > > > > > > > > > > > > Actually, the NVPTX and SPIR targets never support
> > > > > > > > > > > > > VLAs. So I felt like it would be more correct to make
> > > > > > > > > > > > > this a global property of the target.
> > > > > > > > > > > > >
> > > > > > > > > > > > > The difference is that the other programming models
> > > > > > > > > > > > > (OpenCL and CUDA) error out immediatelyand regardless
> > > > > > > > > > > > > of the target because this limitation is reflected in
> > > > > > > > > > > > > the standards that disallow VLAs (see SemaType.cpp).
> > > > > > > > > > > > > For OpenMP we might have target devices that support
> > > > > > > > > > > > > VLA so we shouldn't error out for those.
> > > > > > > > > > > > If you want to make it a global property of the target,
> > > > > > > > > > > > that's fine, but then I don't understand why your
> > > > > > > > > > > > diagnostic only fires when
> > > > > > > > > > > > (S.isInOpenMPDeclareTargetContext() ||
> > > > > > > > > > > > S.isInOpenMPTargetExecutionDirective()).
> > > > > > > > > > > That is because of how OpenMP offloading works and how it
> > > > > > > > > > > is implemented in Clang. Consider the following snippet
> > > > > > > > > > > from the added test case:
> > > > > > > > > > > ```lang=c
> > > > > > > > > > > int vla[arg];
> > > > > > > > > > >
> > > > > > > > > > > #pragma omp target map(vla[0:arg])
> > > > > > > > > > > {
> > > > > > > > > > >// more code here...
> > > > > > > > > > > }
> > > > > > > > > > > ```
> > > > > > > > > > >
> > > > > > > > > > > Clang will take the following steps to compile this into
> > > > > > > > > > > a working binary for a GPU:
> > > > > > > > > > > 1. Parse and (semantically) analyze the code as-is for
> > > > > > > > > > > the host and produce LLVM Bitcode.
> > > > > > > > > > > 2. Parse and analyze again the code as-is and generate
> > > > > > > > > > > code for the offloading target, the GPU in this case.
> > > > > > > > > > > 3. Take LLVM Bitcode from 1., generate host binary and
> > > > > > > > > > > embed target binary from 3.
> > > > > > > > > > >
> > > > > > > > > > > `OpenMPIsDevice` will be true for 2., but the complete
> > > > > > > > > > > source code is analyzed. So to not throw errors for the
> > > > > > > > > > > host code, we have to make sure that we are actually
> > > > > > > > > > > generating code for the target device. This is either in
> > > > > > > > > > > a `target` directive or in a `declare target` region.
> > > > > > > > > > > Note that this is quite similar to what CUDA does, only
> > > > > > > > > > > they have `CUDADiagIfDeviceCode` for this logic. If you
> > > > > > > > > > > want me to add something of that kind for OpenMP target
> > > > > > > > > > > devices, I'm fine with that. However for the given case,
> > > > > > > > > > > it's a bit different because this error should only be
> > > > > > > > > > > thrown for target devices that don't support VLAs...
> > > > > > > > > > I see. So the entire translation unit is re-parsed and
> > > > > > > > > > re-Sema'ed from scratch for the target? Which means you
> > > > > > > > > > need to avoid generating errors about things in the outer
> > > > > > > > > > translation unit that aren't part of the target directive
> > > > > > > > > > that you actually want to compile. I would've expected
> > > > > > > > > > there to be some existing mechanism for that, to be honest,
> > > > > > > > > > as opposed to explicitly trying to suppress target-specific
> > > > > > > > > > diagnostics one by one.
> > > > > > > > > Yes, that is my understanding. For errors, we don't need to
> > > > > > > > > take anything special as the first `cc1` invocation will exit
> > > > > > > > > with a non-zero status so that the driver stops the
> > > > > > > > > compilation. For warnings,
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
Hahnfeld added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
rjmccall wrote:
> Hahnfeld wrote:
> > rjmccall wrote:
> > > Hahnfeld wrote:
> > > > rjmccall wrote:
> > > > > ABataev wrote:
> > > > > > ABataev wrote:
> > > > > > > Hahnfeld wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > The way you've written this makes it sound like "does
> > > > > > > > > > > > > the target support VLAs?", but the actual semantic
> > > > > > > > > > > > > checks treat it as "do OpenMP devices on this target
> > > > > > > > > > > > > support VLAs?" Maybe there should be a more specific
> > > > > > > > > > > > > way to query things about OpenMP devices instead of
> > > > > > > > > > > > > setting a global flag for the target?
> > > > > > > > > > > > Actually, the NVPTX and SPIR targets never support
> > > > > > > > > > > > VLAs. So I felt like it would be more correct to make
> > > > > > > > > > > > this a global property of the target.
> > > > > > > > > > > >
> > > > > > > > > > > > The difference is that the other programming models
> > > > > > > > > > > > (OpenCL and CUDA) error out immediatelyand regardless
> > > > > > > > > > > > of the target because this limitation is reflected in
> > > > > > > > > > > > the standards that disallow VLAs (see SemaType.cpp).
> > > > > > > > > > > > For OpenMP we might have target devices that support
> > > > > > > > > > > > VLA so we shouldn't error out for those.
> > > > > > > > > > > If you want to make it a global property of the target,
> > > > > > > > > > > that's fine, but then I don't understand why your
> > > > > > > > > > > diagnostic only fires when
> > > > > > > > > > > (S.isInOpenMPDeclareTargetContext() ||
> > > > > > > > > > > S.isInOpenMPTargetExecutionDirective()).
> > > > > > > > > > That is because of how OpenMP offloading works and how it
> > > > > > > > > > is implemented in Clang. Consider the following snippet
> > > > > > > > > > from the added test case:
> > > > > > > > > > ```lang=c
> > > > > > > > > > int vla[arg];
> > > > > > > > > >
> > > > > > > > > > #pragma omp target map(vla[0:arg])
> > > > > > > > > > {
> > > > > > > > > >// more code here...
> > > > > > > > > > }
> > > > > > > > > > ```
> > > > > > > > > >
> > > > > > > > > > Clang will take the following steps to compile this into a
> > > > > > > > > > working binary for a GPU:
> > > > > > > > > > 1. Parse and (semantically) analyze the code as-is for the
> > > > > > > > > > host and produce LLVM Bitcode.
> > > > > > > > > > 2. Parse and analyze again the code as-is and generate code
> > > > > > > > > > for the offloading target, the GPU in this case.
> > > > > > > > > > 3. Take LLVM Bitcode from 1., generate host binary and
> > > > > > > > > > embed target binary from 3.
> > > > > > > > > >
> > > > > > > > > > `OpenMPIsDevice` will be true for 2., but the complete
> > > > > > > > > > source code is analyzed. So to not throw errors for the
> > > > > > > > > > host code, we have to make sure that we are actually
> > > > > > > > > > generating code for the target device. This is either in a
> > > > > > > > > > `target` directive or in a `declare target` region.
> > > > > > > > > > Note that this is quite similar to what CUDA does, only
> > > > > > > > > > they have `CUDADiagIfDeviceCode` for this logic. If you
> > > > > > > > > > want me to add something of that kind for OpenMP target
> > > > > > > > > > devices, I'm fine with that. However for the given case,
> > > > > > > > > > it's a bit different because this error should only be
> > > > > > > > > > thrown for target devices that don't support VLAs...
> > > > > > > > > I see. So the entire translation unit is re-parsed and
> > > > > > > > > re-Sema'ed from scratch for the target? Which means you need
> > > > > > > > > to avoid generating errors about things in the outer
> > > > > > > > > translation unit that aren't part of the target directive
> > > > > > > > > that you actually want to compile. I would've expected there
> > > > > > > > > to be some existing mechanism for that, to be honest, as
> > > > > > > > > opposed to explicitly trying to suppress target-specific
> > > > > > > > > diagnostics one by one.
> > > > > > > > Yes, that is my understanding. For errors, we don't need to
> > > > > > > > take anything special as the first `cc1` invocation will exit
> > > > > > > > with a non-zero status so that the driver stops the
> > > > > > > > compilation. For warnings, there seems to be no mechanism in
> > > > > > > > place as I see them duplicated, even in code that is not
> > > > > > > > generate for the target device (verified with an unused
> > > > > > > >
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
rjmccall added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
Hahnfeld wrote:
> rjmccall wrote:
> > Hahnfeld wrote:
> > > rjmccall wrote:
> > > > ABataev wrote:
> > > > > ABataev wrote:
> > > > > > Hahnfeld wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > Hahnfeld wrote:
> > > > > > > > > rjmccall wrote:
> > > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > The way you've written this makes it sound like "does
> > > > > > > > > > > > the target support VLAs?", but the actual semantic
> > > > > > > > > > > > checks treat it as "do OpenMP devices on this target
> > > > > > > > > > > > support VLAs?" Maybe there should be a more specific
> > > > > > > > > > > > way to query things about OpenMP devices instead of
> > > > > > > > > > > > setting a global flag for the target?
> > > > > > > > > > > Actually, the NVPTX and SPIR targets never support VLAs.
> > > > > > > > > > > So I felt like it would be more correct to make this a
> > > > > > > > > > > global property of the target.
> > > > > > > > > > >
> > > > > > > > > > > The difference is that the other programming models
> > > > > > > > > > > (OpenCL and CUDA) error out immediatelyand regardless of
> > > > > > > > > > > the target because this limitation is reflected in the
> > > > > > > > > > > standards that disallow VLAs (see SemaType.cpp). For
> > > > > > > > > > > OpenMP we might have target devices that support VLA so
> > > > > > > > > > > we shouldn't error out for those.
> > > > > > > > > > If you want to make it a global property of the target,
> > > > > > > > > > that's fine, but then I don't understand why your
> > > > > > > > > > diagnostic only fires when
> > > > > > > > > > (S.isInOpenMPDeclareTargetContext() ||
> > > > > > > > > > S.isInOpenMPTargetExecutionDirective()).
> > > > > > > > > That is because of how OpenMP offloading works and how it is
> > > > > > > > > implemented in Clang. Consider the following snippet from the
> > > > > > > > > added test case:
> > > > > > > > > ```lang=c
> > > > > > > > > int vla[arg];
> > > > > > > > >
> > > > > > > > > #pragma omp target map(vla[0:arg])
> > > > > > > > > {
> > > > > > > > >// more code here...
> > > > > > > > > }
> > > > > > > > > ```
> > > > > > > > >
> > > > > > > > > Clang will take the following steps to compile this into a
> > > > > > > > > working binary for a GPU:
> > > > > > > > > 1. Parse and (semantically) analyze the code as-is for the
> > > > > > > > > host and produce LLVM Bitcode.
> > > > > > > > > 2. Parse and analyze again the code as-is and generate code
> > > > > > > > > for the offloading target, the GPU in this case.
> > > > > > > > > 3. Take LLVM Bitcode from 1., generate host binary and embed
> > > > > > > > > target binary from 3.
> > > > > > > > >
> > > > > > > > > `OpenMPIsDevice` will be true for 2., but the complete source
> > > > > > > > > code is analyzed. So to not throw errors for the host code,
> > > > > > > > > we have to make sure that we are actually generating code for
> > > > > > > > > the target device. This is either in a `target` directive or
> > > > > > > > > in a `declare target` region.
> > > > > > > > > Note that this is quite similar to what CUDA does, only they
> > > > > > > > > have `CUDADiagIfDeviceCode` for this logic. If you want me to
> > > > > > > > > add something of that kind for OpenMP target devices, I'm
> > > > > > > > > fine with that. However for the given case, it's a bit
> > > > > > > > > different because this error should only be thrown for target
> > > > > > > > > devices that don't support VLAs...
> > > > > > > > I see. So the entire translation unit is re-parsed and
> > > > > > > > re-Sema'ed from scratch for the target? Which means you need
> > > > > > > > to avoid generating errors about things in the outer
> > > > > > > > translation unit that aren't part of the target directive that
> > > > > > > > you actually want to compile. I would've expected there to be
> > > > > > > > some existing mechanism for that, to be honest, as opposed to
> > > > > > > > explicitly trying to suppress target-specific diagnostics one
> > > > > > > > by one.
> > > > > > > Yes, that is my understanding. For errors, we don't need to take
> > > > > > > anything special as the first `cc1` invocation will exit with a
> > > > > > > non-zero status so that the driver stops the compilation. For
> > > > > > > warnings, there seems to be no mechanism in place as I see them
> > > > > > > duplicated, even in code that is not generate for the target
> > > > > > > device (verified with an unused variable).
> > > > > > >
> > > > > > > @ABataev @gtbercea Do I miss something here?
> > > > > > I'm not aware of any.
> > > > > John, target-specific checks require some special flags (like
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
Hahnfeld added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
rjmccall wrote:
> Hahnfeld wrote:
> > rjmccall wrote:
> > > ABataev wrote:
> > > > ABataev wrote:
> > > > > Hahnfeld wrote:
> > > > > > rjmccall wrote:
> > > > > > > Hahnfeld wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > Hahnfeld wrote:
> > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > The way you've written this makes it sound like "does the
> > > > > > > > > > > target support VLAs?", but the actual semantic checks
> > > > > > > > > > > treat it as "do OpenMP devices on this target support
> > > > > > > > > > > VLAs?" Maybe there should be a more specific way to
> > > > > > > > > > > query things about OpenMP devices instead of setting a
> > > > > > > > > > > global flag for the target?
> > > > > > > > > > Actually, the NVPTX and SPIR targets never support VLAs. So
> > > > > > > > > > I felt like it would be more correct to make this a global
> > > > > > > > > > property of the target.
> > > > > > > > > >
> > > > > > > > > > The difference is that the other programming models (OpenCL
> > > > > > > > > > and CUDA) error out immediatelyand regardless of the target
> > > > > > > > > > because this limitation is reflected in the standards that
> > > > > > > > > > disallow VLAs (see SemaType.cpp). For OpenMP we might have
> > > > > > > > > > target devices that support VLA so we shouldn't error out
> > > > > > > > > > for those.
> > > > > > > > > If you want to make it a global property of the target,
> > > > > > > > > that's fine, but then I don't understand why your diagnostic
> > > > > > > > > only fires when (S.isInOpenMPDeclareTargetContext() ||
> > > > > > > > > S.isInOpenMPTargetExecutionDirective()).
> > > > > > > > That is because of how OpenMP offloading works and how it is
> > > > > > > > implemented in Clang. Consider the following snippet from the
> > > > > > > > added test case:
> > > > > > > > ```lang=c
> > > > > > > > int vla[arg];
> > > > > > > >
> > > > > > > > #pragma omp target map(vla[0:arg])
> > > > > > > > {
> > > > > > > >// more code here...
> > > > > > > > }
> > > > > > > > ```
> > > > > > > >
> > > > > > > > Clang will take the following steps to compile this into a
> > > > > > > > working binary for a GPU:
> > > > > > > > 1. Parse and (semantically) analyze the code as-is for the host
> > > > > > > > and produce LLVM Bitcode.
> > > > > > > > 2. Parse and analyze again the code as-is and generate code for
> > > > > > > > the offloading target, the GPU in this case.
> > > > > > > > 3. Take LLVM Bitcode from 1., generate host binary and embed
> > > > > > > > target binary from 3.
> > > > > > > >
> > > > > > > > `OpenMPIsDevice` will be true for 2., but the complete source
> > > > > > > > code is analyzed. So to not throw errors for the host code, we
> > > > > > > > have to make sure that we are actually generating code for the
> > > > > > > > target device. This is either in a `target` directive or in a
> > > > > > > > `declare target` region.
> > > > > > > > Note that this is quite similar to what CUDA does, only they
> > > > > > > > have `CUDADiagIfDeviceCode` for this logic. If you want me to
> > > > > > > > add something of that kind for OpenMP target devices, I'm fine
> > > > > > > > with that. However for the given case, it's a bit different
> > > > > > > > because this error should only be thrown for target devices
> > > > > > > > that don't support VLAs...
> > > > > > > I see. So the entire translation unit is re-parsed and
> > > > > > > re-Sema'ed from scratch for the target? Which means you need to
> > > > > > > avoid generating errors about things in the outer translation
> > > > > > > unit that aren't part of the target directive that you actually
> > > > > > > want to compile. I would've expected there to be some existing
> > > > > > > mechanism for that, to be honest, as opposed to explicitly trying
> > > > > > > to suppress target-specific diagnostics one by one.
> > > > > > Yes, that is my understanding. For errors, we don't need to take
> > > > > > anything special as the first `cc1` invocation will exit with a
> > > > > > non-zero status so that the driver stops the compilation. For
> > > > > > warnings, there seems to be no mechanism in place as I see them
> > > > > > duplicated, even in code that is not generate for the target device
> > > > > > (verified with an unused variable).
> > > > > >
> > > > > > @ABataev @gtbercea Do I miss something here?
> > > > > I'm not aware of any.
> > > > John, target-specific checks require some special flags (like
> > > > LangOpts.Cuda) that are not set when we re-compile the code for OpenMP
> > > > devices. That's why errors are not emitted for the non-target code. But
> > > > also because of that, we need some special
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
rjmccall added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
Hahnfeld wrote:
> rjmccall wrote:
> > ABataev wrote:
> > > ABataev wrote:
> > > > Hahnfeld wrote:
> > > > > rjmccall wrote:
> > > > > > Hahnfeld wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > Hahnfeld wrote:
> > > > > > > > > rjmccall wrote:
> > > > > > > > > > The way you've written this makes it sound like "does the
> > > > > > > > > > target support VLAs?", but the actual semantic checks treat
> > > > > > > > > > it as "do OpenMP devices on this target support VLAs?"
> > > > > > > > > > Maybe there should be a more specific way to query things
> > > > > > > > > > about OpenMP devices instead of setting a global flag for
> > > > > > > > > > the target?
> > > > > > > > > Actually, the NVPTX and SPIR targets never support VLAs. So I
> > > > > > > > > felt like it would be more correct to make this a global
> > > > > > > > > property of the target.
> > > > > > > > >
> > > > > > > > > The difference is that the other programming models (OpenCL
> > > > > > > > > and CUDA) error out immediatelyand regardless of the target
> > > > > > > > > because this limitation is reflected in the standards that
> > > > > > > > > disallow VLAs (see SemaType.cpp). For OpenMP we might have
> > > > > > > > > target devices that support VLA so we shouldn't error out for
> > > > > > > > > those.
> > > > > > > > If you want to make it a global property of the target, that's
> > > > > > > > fine, but then I don't understand why your diagnostic only
> > > > > > > > fires when (S.isInOpenMPDeclareTargetContext() ||
> > > > > > > > S.isInOpenMPTargetExecutionDirective()).
> > > > > > > That is because of how OpenMP offloading works and how it is
> > > > > > > implemented in Clang. Consider the following snippet from the
> > > > > > > added test case:
> > > > > > > ```lang=c
> > > > > > > int vla[arg];
> > > > > > >
> > > > > > > #pragma omp target map(vla[0:arg])
> > > > > > > {
> > > > > > >// more code here...
> > > > > > > }
> > > > > > > ```
> > > > > > >
> > > > > > > Clang will take the following steps to compile this into a
> > > > > > > working binary for a GPU:
> > > > > > > 1. Parse and (semantically) analyze the code as-is for the host
> > > > > > > and produce LLVM Bitcode.
> > > > > > > 2. Parse and analyze again the code as-is and generate code for
> > > > > > > the offloading target, the GPU in this case.
> > > > > > > 3. Take LLVM Bitcode from 1., generate host binary and embed
> > > > > > > target binary from 3.
> > > > > > >
> > > > > > > `OpenMPIsDevice` will be true for 2., but the complete source
> > > > > > > code is analyzed. So to not throw errors for the host code, we
> > > > > > > have to make sure that we are actually generating code for the
> > > > > > > target device. This is either in a `target` directive or in a
> > > > > > > `declare target` region.
> > > > > > > Note that this is quite similar to what CUDA does, only they have
> > > > > > > `CUDADiagIfDeviceCode` for this logic. If you want me to add
> > > > > > > something of that kind for OpenMP target devices, I'm fine with
> > > > > > > that. However for the given case, it's a bit different because
> > > > > > > this error should only be thrown for target devices that don't
> > > > > > > support VLAs...
> > > > > > I see. So the entire translation unit is re-parsed and re-Sema'ed
> > > > > > from scratch for the target? Which means you need to avoid
> > > > > > generating errors about things in the outer translation unit that
> > > > > > aren't part of the target directive that you actually want to
> > > > > > compile. I would've expected there to be some existing mechanism
> > > > > > for that, to be honest, as opposed to explicitly trying to suppress
> > > > > > target-specific diagnostics one by one.
> > > > > Yes, that is my understanding. For errors, we don't need to take
> > > > > anything special as the first `cc1` invocation will exit with a
> > > > > non-zero status so that the driver stops the compilation. For
> > > > > warnings, there seems to be no mechanism in place as I see them
> > > > > duplicated, even in code that is not generate for the target device
> > > > > (verified with an unused variable).
> > > > >
> > > > > @ABataev @gtbercea Do I miss something here?
> > > > I'm not aware of any.
> > > John, target-specific checks require some special flags (like
> > > LangOpts.Cuda) that are not set when we re-compile the code for OpenMP
> > > devices. That's why errors are not emitted for the non-target code. But
> > > also because of that, we need some special OpenMP checks for
> > > target-specific code inside the target regions. For example, code in
> > > lib/Sema/SemaType.cpp, lines 2184, 2185 (see this file in this patch)
> > >
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
Hahnfeld added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
rjmccall wrote:
> ABataev wrote:
> > ABataev wrote:
> > > Hahnfeld wrote:
> > > > rjmccall wrote:
> > > > > Hahnfeld wrote:
> > > > > > rjmccall wrote:
> > > > > > > Hahnfeld wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > The way you've written this makes it sound like "does the
> > > > > > > > > target support VLAs?", but the actual semantic checks treat
> > > > > > > > > it as "do OpenMP devices on this target support VLAs?" Maybe
> > > > > > > > > there should be a more specific way to query things about
> > > > > > > > > OpenMP devices instead of setting a global flag for the
> > > > > > > > > target?
> > > > > > > > Actually, the NVPTX and SPIR targets never support VLAs. So I
> > > > > > > > felt like it would be more correct to make this a global
> > > > > > > > property of the target.
> > > > > > > >
> > > > > > > > The difference is that the other programming models (OpenCL and
> > > > > > > > CUDA) error out immediatelyand regardless of the target because
> > > > > > > > this limitation is reflected in the standards that disallow
> > > > > > > > VLAs (see SemaType.cpp). For OpenMP we might have target
> > > > > > > > devices that support VLA so we shouldn't error out for those.
> > > > > > > If you want to make it a global property of the target, that's
> > > > > > > fine, but then I don't understand why your diagnostic only fires
> > > > > > > when (S.isInOpenMPDeclareTargetContext() ||
> > > > > > > S.isInOpenMPTargetExecutionDirective()).
> > > > > > That is because of how OpenMP offloading works and how it is
> > > > > > implemented in Clang. Consider the following snippet from the added
> > > > > > test case:
> > > > > > ```lang=c
> > > > > > int vla[arg];
> > > > > >
> > > > > > #pragma omp target map(vla[0:arg])
> > > > > > {
> > > > > >// more code here...
> > > > > > }
> > > > > > ```
> > > > > >
> > > > > > Clang will take the following steps to compile this into a working
> > > > > > binary for a GPU:
> > > > > > 1. Parse and (semantically) analyze the code as-is for the host and
> > > > > > produce LLVM Bitcode.
> > > > > > 2. Parse and analyze again the code as-is and generate code for the
> > > > > > offloading target, the GPU in this case.
> > > > > > 3. Take LLVM Bitcode from 1., generate host binary and embed target
> > > > > > binary from 3.
> > > > > >
> > > > > > `OpenMPIsDevice` will be true for 2., but the complete source code
> > > > > > is analyzed. So to not throw errors for the host code, we have to
> > > > > > make sure that we are actually generating code for the target
> > > > > > device. This is either in a `target` directive or in a `declare
> > > > > > target` region.
> > > > > > Note that this is quite similar to what CUDA does, only they have
> > > > > > `CUDADiagIfDeviceCode` for this logic. If you want me to add
> > > > > > something of that kind for OpenMP target devices, I'm fine with
> > > > > > that. However for the given case, it's a bit different because this
> > > > > > error should only be thrown for target devices that don't support
> > > > > > VLAs...
> > > > > I see. So the entire translation unit is re-parsed and re-Sema'ed
> > > > > from scratch for the target? Which means you need to avoid
> > > > > generating errors about things in the outer translation unit that
> > > > > aren't part of the target directive that you actually want to
> > > > > compile. I would've expected there to be some existing mechanism for
> > > > > that, to be honest, as opposed to explicitly trying to suppress
> > > > > target-specific diagnostics one by one.
> > > > Yes, that is my understanding. For errors, we don't need to take
> > > > anything special as the first `cc1` invocation will exit with a
> > > > non-zero status so that the driver stops the compilation. For warnings,
> > > > there seems to be no mechanism in place as I see them duplicated, even
> > > > in code that is not generate for the target device (verified with an
> > > > unused variable).
> > > >
> > > > @ABataev @gtbercea Do I miss something here?
> > > I'm not aware of any.
> > John, target-specific checks require some special flags (like
> > LangOpts.Cuda) that are not set when we re-compile the code for OpenMP
> > devices. That's why errors are not emitted for the non-target code. But
> > also because of that, we need some special OpenMP checks for
> > target-specific code inside the target regions. For example, code in
> > lib/Sema/SemaType.cpp, lines 2184, 2185 (see this file in this patch)
> > checks for Cuda compilation and prohibits using of VLAs in Cuda mode. We
> > also should prohibit using of VLAs in target code for NVPTX devices or
> > other devices that do not support VLAs in OpenMP
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
rjmccall added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
ABataev wrote:
> ABataev wrote:
> > Hahnfeld wrote:
> > > rjmccall wrote:
> > > > Hahnfeld wrote:
> > > > > rjmccall wrote:
> > > > > > Hahnfeld wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > The way you've written this makes it sound like "does the
> > > > > > > > target support VLAs?", but the actual semantic checks treat it
> > > > > > > > as "do OpenMP devices on this target support VLAs?" Maybe
> > > > > > > > there should be a more specific way to query things about
> > > > > > > > OpenMP devices instead of setting a global flag for the target?
> > > > > > > Actually, the NVPTX and SPIR targets never support VLAs. So I
> > > > > > > felt like it would be more correct to make this a global property
> > > > > > > of the target.
> > > > > > >
> > > > > > > The difference is that the other programming models (OpenCL and
> > > > > > > CUDA) error out immediatelyand regardless of the target because
> > > > > > > this limitation is reflected in the standards that disallow VLAs
> > > > > > > (see SemaType.cpp). For OpenMP we might have target devices that
> > > > > > > support VLA so we shouldn't error out for those.
> > > > > > If you want to make it a global property of the target, that's
> > > > > > fine, but then I don't understand why your diagnostic only fires
> > > > > > when (S.isInOpenMPDeclareTargetContext() ||
> > > > > > S.isInOpenMPTargetExecutionDirective()).
> > > > > That is because of how OpenMP offloading works and how it is
> > > > > implemented in Clang. Consider the following snippet from the added
> > > > > test case:
> > > > > ```lang=c
> > > > > int vla[arg];
> > > > >
> > > > > #pragma omp target map(vla[0:arg])
> > > > > {
> > > > >// more code here...
> > > > > }
> > > > > ```
> > > > >
> > > > > Clang will take the following steps to compile this into a working
> > > > > binary for a GPU:
> > > > > 1. Parse and (semantically) analyze the code as-is for the host and
> > > > > produce LLVM Bitcode.
> > > > > 2. Parse and analyze again the code as-is and generate code for the
> > > > > offloading target, the GPU in this case.
> > > > > 3. Take LLVM Bitcode from 1., generate host binary and embed target
> > > > > binary from 3.
> > > > >
> > > > > `OpenMPIsDevice` will be true for 2., but the complete source code is
> > > > > analyzed. So to not throw errors for the host code, we have to make
> > > > > sure that we are actually generating code for the target device. This
> > > > > is either in a `target` directive or in a `declare target` region.
> > > > > Note that this is quite similar to what CUDA does, only they have
> > > > > `CUDADiagIfDeviceCode` for this logic. If you want me to add
> > > > > something of that kind for OpenMP target devices, I'm fine with that.
> > > > > However for the given case, it's a bit different because this error
> > > > > should only be thrown for target devices that don't support VLAs...
> > > > I see. So the entire translation unit is re-parsed and re-Sema'ed from
> > > > scratch for the target? Which means you need to avoid generating
> > > > errors about things in the outer translation unit that aren't part of
> > > > the target directive that you actually want to compile. I would've
> > > > expected there to be some existing mechanism for that, to be honest, as
> > > > opposed to explicitly trying to suppress target-specific diagnostics
> > > > one by one.
> > > Yes, that is my understanding. For errors, we don't need to take anything
> > > special as the first `cc1` invocation will exit with a non-zero status so
> > > that the driver stops the compilation. For warnings, there seems to be no
> > > mechanism in place as I see them duplicated, even in code that is not
> > > generate for the target device (verified with an unused variable).
> > >
> > > @ABataev @gtbercea Do I miss something here?
> > I'm not aware of any.
> John, target-specific checks require some special flags (like LangOpts.Cuda)
> that are not set when we re-compile the code for OpenMP devices. That's why
> errors are not emitted for the non-target code. But also because of that, we
> need some special OpenMP checks for target-specific code inside the target
> regions. For example, code in lib/Sema/SemaType.cpp, lines 2184, 2185 (see
> this file in this patch) checks for Cuda compilation and prohibits using of
> VLAs in Cuda mode. We also should prohibit using of VLAs in target code for
> NVPTX devices or other devices that do not support VLAs in OpenMP mode.
I think it would be cleaner here, and better for our OpenMP support overall, if
we found a more general way to suppress unwanted diagnostics in the second
invocation for code outside of the target directive. This check (and
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
ABataev added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
ABataev wrote:
> Hahnfeld wrote:
> > rjmccall wrote:
> > > Hahnfeld wrote:
> > > > rjmccall wrote:
> > > > > Hahnfeld wrote:
> > > > > > rjmccall wrote:
> > > > > > > The way you've written this makes it sound like "does the target
> > > > > > > support VLAs?", but the actual semantic checks treat it as "do
> > > > > > > OpenMP devices on this target support VLAs?" Maybe there should
> > > > > > > be a more specific way to query things about OpenMP devices
> > > > > > > instead of setting a global flag for the target?
> > > > > > Actually, the NVPTX and SPIR targets never support VLAs. So I felt
> > > > > > like it would be more correct to make this a global property of the
> > > > > > target.
> > > > > >
> > > > > > The difference is that the other programming models (OpenCL and
> > > > > > CUDA) error out immediatelyand regardless of the target because
> > > > > > this limitation is reflected in the standards that disallow VLAs
> > > > > > (see SemaType.cpp). For OpenMP we might have target devices that
> > > > > > support VLA so we shouldn't error out for those.
> > > > > If you want to make it a global property of the target, that's fine,
> > > > > but then I don't understand why your diagnostic only fires when
> > > > > (S.isInOpenMPDeclareTargetContext() ||
> > > > > S.isInOpenMPTargetExecutionDirective()).
> > > > That is because of how OpenMP offloading works and how it is
> > > > implemented in Clang. Consider the following snippet from the added
> > > > test case:
> > > > ```lang=c
> > > > int vla[arg];
> > > >
> > > > #pragma omp target map(vla[0:arg])
> > > > {
> > > >// more code here...
> > > > }
> > > > ```
> > > >
> > > > Clang will take the following steps to compile this into a working
> > > > binary for a GPU:
> > > > 1. Parse and (semantically) analyze the code as-is for the host and
> > > > produce LLVM Bitcode.
> > > > 2. Parse and analyze again the code as-is and generate code for the
> > > > offloading target, the GPU in this case.
> > > > 3. Take LLVM Bitcode from 1., generate host binary and embed target
> > > > binary from 3.
> > > >
> > > > `OpenMPIsDevice` will be true for 2., but the complete source code is
> > > > analyzed. So to not throw errors for the host code, we have to make
> > > > sure that we are actually generating code for the target device. This
> > > > is either in a `target` directive or in a `declare target` region.
> > > > Note that this is quite similar to what CUDA does, only they have
> > > > `CUDADiagIfDeviceCode` for this logic. If you want me to add something
> > > > of that kind for OpenMP target devices, I'm fine with that. However for
> > > > the given case, it's a bit different because this error should only be
> > > > thrown for target devices that don't support VLAs...
> > > I see. So the entire translation unit is re-parsed and re-Sema'ed from
> > > scratch for the target? Which means you need to avoid generating errors
> > > about things in the outer translation unit that aren't part of the target
> > > directive that you actually want to compile. I would've expected there
> > > to be some existing mechanism for that, to be honest, as opposed to
> > > explicitly trying to suppress target-specific diagnostics one by one.
> > Yes, that is my understanding. For errors, we don't need to take anything
> > special as the first `cc1` invocation will exit with a non-zero status so
> > that the driver stops the compilation. For warnings, there seems to be no
> > mechanism in place as I see them duplicated, even in code that is not
> > generate for the target device (verified with an unused variable).
> >
> > @ABataev @gtbercea Do I miss something here?
> I'm not aware of any.
John, target-specific checks require some special flags (like LangOpts.Cuda)
that are not set when we re-compile the code for OpenMP devices. That's why
errors are not emitted for the non-target code. But also because of that, we
need some special OpenMP checks for target-specific code inside the target
regions. For example, code in lib/Sema/SemaType.cpp, lines 2184, 2185 (see this
file in this patch) checks for Cuda compilation and prohibits using of VLAs in
Cuda mode. We also should prohibit using of VLAs in target code for NVPTX
devices or other devices that do not support VLAs in OpenMP mode.
https://reviews.llvm.org/D39505
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
ABataev added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
Hahnfeld wrote:
> rjmccall wrote:
> > Hahnfeld wrote:
> > > rjmccall wrote:
> > > > Hahnfeld wrote:
> > > > > rjmccall wrote:
> > > > > > The way you've written this makes it sound like "does the target
> > > > > > support VLAs?", but the actual semantic checks treat it as "do
> > > > > > OpenMP devices on this target support VLAs?" Maybe there should be
> > > > > > a more specific way to query things about OpenMP devices instead of
> > > > > > setting a global flag for the target?
> > > > > Actually, the NVPTX and SPIR targets never support VLAs. So I felt
> > > > > like it would be more correct to make this a global property of the
> > > > > target.
> > > > >
> > > > > The difference is that the other programming models (OpenCL and CUDA)
> > > > > error out immediatelyand regardless of the target because this
> > > > > limitation is reflected in the standards that disallow VLAs (see
> > > > > SemaType.cpp). For OpenMP we might have target devices that support
> > > > > VLA so we shouldn't error out for those.
> > > > If you want to make it a global property of the target, that's fine,
> > > > but then I don't understand why your diagnostic only fires when
> > > > (S.isInOpenMPDeclareTargetContext() ||
> > > > S.isInOpenMPTargetExecutionDirective()).
> > > That is because of how OpenMP offloading works and how it is implemented
> > > in Clang. Consider the following snippet from the added test case:
> > > ```lang=c
> > > int vla[arg];
> > >
> > > #pragma omp target map(vla[0:arg])
> > > {
> > >// more code here...
> > > }
> > > ```
> > >
> > > Clang will take the following steps to compile this into a working binary
> > > for a GPU:
> > > 1. Parse and (semantically) analyze the code as-is for the host and
> > > produce LLVM Bitcode.
> > > 2. Parse and analyze again the code as-is and generate code for the
> > > offloading target, the GPU in this case.
> > > 3. Take LLVM Bitcode from 1., generate host binary and embed target
> > > binary from 3.
> > >
> > > `OpenMPIsDevice` will be true for 2., but the complete source code is
> > > analyzed. So to not throw errors for the host code, we have to make sure
> > > that we are actually generating code for the target device. This is
> > > either in a `target` directive or in a `declare target` region.
> > > Note that this is quite similar to what CUDA does, only they have
> > > `CUDADiagIfDeviceCode` for this logic. If you want me to add something of
> > > that kind for OpenMP target devices, I'm fine with that. However for the
> > > given case, it's a bit different because this error should only be thrown
> > > for target devices that don't support VLAs...
> > I see. So the entire translation unit is re-parsed and re-Sema'ed from
> > scratch for the target? Which means you need to avoid generating errors
> > about things in the outer translation unit that aren't part of the target
> > directive that you actually want to compile. I would've expected there to
> > be some existing mechanism for that, to be honest, as opposed to explicitly
> > trying to suppress target-specific diagnostics one by one.
> Yes, that is my understanding. For errors, we don't need to take anything
> special as the first `cc1` invocation will exit with a non-zero status so
> that the driver stops the compilation. For warnings, there seems to be no
> mechanism in place as I see them duplicated, even in code that is not
> generate for the target device (verified with an unused variable).
>
> @ABataev @gtbercea Do I miss something here?
I'm not aware of any.
https://reviews.llvm.org/D39505
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
Hahnfeld marked an inline comment as done.
Hahnfeld added a subscriber: gtbercea.
Hahnfeld added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
rjmccall wrote:
> Hahnfeld wrote:
> > rjmccall wrote:
> > > Hahnfeld wrote:
> > > > rjmccall wrote:
> > > > > The way you've written this makes it sound like "does the target
> > > > > support VLAs?", but the actual semantic checks treat it as "do OpenMP
> > > > > devices on this target support VLAs?" Maybe there should be a more
> > > > > specific way to query things about OpenMP devices instead of setting
> > > > > a global flag for the target?
> > > > Actually, the NVPTX and SPIR targets never support VLAs. So I felt like
> > > > it would be more correct to make this a global property of the target.
> > > >
> > > > The difference is that the other programming models (OpenCL and CUDA)
> > > > error out immediatelyand regardless of the target because this
> > > > limitation is reflected in the standards that disallow VLAs (see
> > > > SemaType.cpp). For OpenMP we might have target devices that support VLA
> > > > so we shouldn't error out for those.
> > > If you want to make it a global property of the target, that's fine, but
> > > then I don't understand why your diagnostic only fires when
> > > (S.isInOpenMPDeclareTargetContext() ||
> > > S.isInOpenMPTargetExecutionDirective()).
> > That is because of how OpenMP offloading works and how it is implemented in
> > Clang. Consider the following snippet from the added test case:
> > ```lang=c
> > int vla[arg];
> >
> > #pragma omp target map(vla[0:arg])
> > {
> >// more code here...
> > }
> > ```
> >
> > Clang will take the following steps to compile this into a working binary
> > for a GPU:
> > 1. Parse and (semantically) analyze the code as-is for the host and produce
> > LLVM Bitcode.
> > 2. Parse and analyze again the code as-is and generate code for the
> > offloading target, the GPU in this case.
> > 3. Take LLVM Bitcode from 1., generate host binary and embed target binary
> > from 3.
> >
> > `OpenMPIsDevice` will be true for 2., but the complete source code is
> > analyzed. So to not throw errors for the host code, we have to make sure
> > that we are actually generating code for the target device. This is either
> > in a `target` directive or in a `declare target` region.
> > Note that this is quite similar to what CUDA does, only they have
> > `CUDADiagIfDeviceCode` for this logic. If you want me to add something of
> > that kind for OpenMP target devices, I'm fine with that. However for the
> > given case, it's a bit different because this error should only be thrown
> > for target devices that don't support VLAs...
> I see. So the entire translation unit is re-parsed and re-Sema'ed from
> scratch for the target? Which means you need to avoid generating errors
> about things in the outer translation unit that aren't part of the target
> directive that you actually want to compile. I would've expected there to be
> some existing mechanism for that, to be honest, as opposed to explicitly
> trying to suppress target-specific diagnostics one by one.
Yes, that is my understanding. For errors, we don't need to take anything
special as the first `cc1` invocation will exit with a non-zero status so that
the driver stops the compilation. For warnings, there seems to be no mechanism
in place as I see them duplicated, even in code that is not generate for the
target device (verified with an unused variable).
@ABataev @gtbercea Do I miss something here?
https://reviews.llvm.org/D39505
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
rjmccall added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
Hahnfeld wrote:
> rjmccall wrote:
> > Hahnfeld wrote:
> > > rjmccall wrote:
> > > > The way you've written this makes it sound like "does the target
> > > > support VLAs?", but the actual semantic checks treat it as "do OpenMP
> > > > devices on this target support VLAs?" Maybe there should be a more
> > > > specific way to query things about OpenMP devices instead of setting a
> > > > global flag for the target?
> > > Actually, the NVPTX and SPIR targets never support VLAs. So I felt like
> > > it would be more correct to make this a global property of the target.
> > >
> > > The difference is that the other programming models (OpenCL and CUDA)
> > > error out immediatelyand regardless of the target because this limitation
> > > is reflected in the standards that disallow VLAs (see SemaType.cpp). For
> > > OpenMP we might have target devices that support VLA so we shouldn't
> > > error out for those.
> > If you want to make it a global property of the target, that's fine, but
> > then I don't understand why your diagnostic only fires when
> > (S.isInOpenMPDeclareTargetContext() ||
> > S.isInOpenMPTargetExecutionDirective()).
> That is because of how OpenMP offloading works and how it is implemented in
> Clang. Consider the following snippet from the added test case:
> ```lang=c
> int vla[arg];
>
> #pragma omp target map(vla[0:arg])
> {
>// more code here...
> }
> ```
>
> Clang will take the following steps to compile this into a working binary for
> a GPU:
> 1. Parse and (semantically) analyze the code as-is for the host and produce
> LLVM Bitcode.
> 2. Parse and analyze again the code as-is and generate code for the
> offloading target, the GPU in this case.
> 3. Take LLVM Bitcode from 1., generate host binary and embed target binary
> from 3.
>
> `OpenMPIsDevice` will be true for 2., but the complete source code is
> analyzed. So to not throw errors for the host code, we have to make sure that
> we are actually generating code for the target device. This is either in a
> `target` directive or in a `declare target` region.
> Note that this is quite similar to what CUDA does, only they have
> `CUDADiagIfDeviceCode` for this logic. If you want me to add something of
> that kind for OpenMP target devices, I'm fine with that. However for the
> given case, it's a bit different because this error should only be thrown for
> target devices that don't support VLAs...
I see. So the entire translation unit is re-parsed and re-Sema'ed from scratch
for the target? Which means you need to avoid generating errors about things
in the outer translation unit that aren't part of the target directive that you
actually want to compile. I would've expected there to be some existing
mechanism for that, to be honest, as opposed to explicitly trying to suppress
target-specific diagnostics one by one.
https://reviews.llvm.org/D39505
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
Hahnfeld marked 3 inline comments as done.
Hahnfeld added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
rjmccall wrote:
> Hahnfeld wrote:
> > rjmccall wrote:
> > > The way you've written this makes it sound like "does the target support
> > > VLAs?", but the actual semantic checks treat it as "do OpenMP devices on
> > > this target support VLAs?" Maybe there should be a more specific way to
> > > query things about OpenMP devices instead of setting a global flag for
> > > the target?
> > Actually, the NVPTX and SPIR targets never support VLAs. So I felt like it
> > would be more correct to make this a global property of the target.
> >
> > The difference is that the other programming models (OpenCL and CUDA) error
> > out immediatelyand regardless of the target because this limitation is
> > reflected in the standards that disallow VLAs (see SemaType.cpp). For
> > OpenMP we might have target devices that support VLA so we shouldn't error
> > out for those.
> If you want to make it a global property of the target, that's fine, but then
> I don't understand why your diagnostic only fires when
> (S.isInOpenMPDeclareTargetContext() ||
> S.isInOpenMPTargetExecutionDirective()).
That is because of how OpenMP offloading works and how it is implemented in
Clang. Consider the following snippet from the added test case:
```lang=c
int vla[arg];
#pragma omp target map(vla[0:arg])
{
// more code here...
}
```
Clang will take the following steps to compile this into a working binary for a
GPU:
1. Parse and (semantically) analyze the code as-is for the host and produce
LLVM Bitcode.
2. Parse and analyze again the code as-is and generate code for the offloading
target, the GPU in this case.
3. Take LLVM Bitcode from 1., generate host binary and embed target binary from
3.
`OpenMPIsDevice` will be true for 2., but the complete source code is analyzed.
So to not throw errors for the host code, we have to make sure that we are
actually generating code for the target device. This is either in a `target`
directive or in a `declare target` region.
Note that this is quite similar to what CUDA does, only they have
`CUDADiagIfDeviceCode` for this logic. If you want me to add something of that
kind for OpenMP target devices, I'm fine with that. However for the given case,
it's a bit different because this error should only be thrown for target
devices that don't support VLAs...
https://reviews.llvm.org/D39505
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
rjmccall added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
Hahnfeld wrote:
> rjmccall wrote:
> > The way you've written this makes it sound like "does the target support
> > VLAs?", but the actual semantic checks treat it as "do OpenMP devices on
> > this target support VLAs?" Maybe there should be a more specific way to
> > query things about OpenMP devices instead of setting a global flag for the
> > target?
> Actually, the NVPTX and SPIR targets never support VLAs. So I felt like it
> would be more correct to make this a global property of the target.
>
> The difference is that the other programming models (OpenCL and CUDA) error
> out immediatelyand regardless of the target because this limitation is
> reflected in the standards that disallow VLAs (see SemaType.cpp). For OpenMP
> we might have target devices that support VLA so we shouldn't error out for
> those.
If you want to make it a global property of the target, that's fine, but then I
don't understand why your diagnostic only fires when
(S.isInOpenMPDeclareTargetContext() || S.isInOpenMPTargetExecutionDirective()).
https://reviews.llvm.org/D39505
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
Hahnfeld added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
rjmccall wrote:
> The way you've written this makes it sound like "does the target support
> VLAs?", but the actual semantic checks treat it as "do OpenMP devices on this
> target support VLAs?" Maybe there should be a more specific way to query
> things about OpenMP devices instead of setting a global flag for the target?
Actually, the NVPTX and SPIR targets never support VLAs. So I felt like it
would be more correct to make this a global property of the target.
The difference is that the other programming models (OpenCL and CUDA) error out
immediatelyand regardless of the target because this limitation is reflected in
the standards that disallow VLAs (see SemaType.cpp). For OpenMP we might have
target devices that support VLA so we shouldn't error out for those.
https://reviews.llvm.org/D39505
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
rjmccall added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:944
+ /// \brief Whether target supports variable-length arrays.
+ bool isVLASupported() const { return VLASupported; }
+
The way you've written this makes it sound like "does the target support
VLAs?", but the actual semantic checks treat it as "do OpenMP devices on this
target support VLAs?" Maybe there should be a more specific way to query
things about OpenMP devices instead of setting a global flag for the target?
https://reviews.llvm.org/D39505
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39505: [OpenMP] Show error if VLAs are not supported
Hahnfeld created this revision.
Herald added a subscriber: jholewinski.
Some target devices (e.g. Nvidia GPUs) don't support dynamic stack
allocation and hence no VLAs. Print errors with description instead
of failing in the backend or generating code that doesn't work.
This patch handles explicit uses of VLAs (local variable in target
or declare target region) or implicitly generated (private) VLAs
for reductions on VLAs or on array sections with non-constant size.
https://reviews.llvm.org/D39505
Files:
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TargetInfo.h
include/clang/Sema/Sema.h
lib/Basic/TargetInfo.cpp
lib/Basic/Targets/NVPTX.cpp
lib/Basic/Targets/SPIR.h
lib/Sema/SemaOpenMP.cpp
lib/Sema/SemaType.cpp
test/OpenMP/target_vla_messages.c
Index: test/OpenMP/target_vla_messages.c
===
--- /dev/null
+++ test/OpenMP/target_vla_messages.c
@@ -0,0 +1,191 @@
+// PowerPC supports VLAs.
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-unknown-unknown -emit-llvm-bc %s -o %t-ppc-host-ppc.bc
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-unknown-unknown -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host-ppc.bc -o %t-ppc-device.ll
+
+// Nvidia GPUs don't support VLAs.
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host-nvptx.bc
+// RUN: %clang_cc1 -verify -DNO_VLA -fopenmp -x c++ -triple nvptx64-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host-nvptx.bc -o %t-nvptx-device.ll
+
+#ifndef NO_VLA
+// expected-no-diagnostics
+#endif
+
+#pragma omp declare target
+void declare(int arg) {
+ int a[2];
+#ifdef NO_VLA
+ // expected-error@+3 {{cannot use variable-length array in a declare target region}}
+ // expected-note@+2 {{the target device does not support allocating variable-length arrays}}
+#endif
+ int vla[arg];
+}
+
+void declare_parallel_reduction(int arg) {
+ int a[2];
+
+#pragma omp parallel reduction(+: a)
+ { }
+
+#pragma omp parallel reduction(+: a[0:2])
+ { }
+
+#ifdef NO_VLA
+ // expected-error@+3 {{cannot generate code for reduction on array section, which requires a variable-length array}}
+ // expected-note@+2 {{the target device does not support allocating variable-length arrays}}
+#endif
+#pragma omp parallel reduction(+: a[0:arg])
+ { }
+}
+#pragma omp end declare target
+
+void target(int arg) {
+#pragma omp target
+ {
+#ifdef NO_VLA
+// expected-error@+3 {{cannot use variable-length array in a target region}}
+// expected-note@+2 {{the target device does not support allocating variable-length arrays}}
+#endif
+int vla[arg];
+ }
+
+#pragma omp target
+ {
+#pragma omp parallel
+{
+#ifdef NO_VLA
+ // expected-error@+3 {{cannot use variable-length array in a target region}}
+ // expected-note@+2 {{the target device does not support allocating variable-length arrays}}
+#endif
+ int vla[arg];
+}
+ }
+}
+
+void teams_reduction(int arg) {
+ int a[2];
+ int vla[arg];
+
+#pragma omp target map(a)
+#pragma omp teams reduction(+: a)
+ { }
+
+#ifdef NO_VLA
+ // expected-error@+4 {{cannot generate code for reduction on variable-length array}}
+ // expected-note@+3 {{the target device does not support allocating variable-length arrays}}
+#endif
+#pragma omp target map(vla)
+#pragma omp teams reduction(+: vla)
+ { }
+
+#pragma omp target map(a[0:2])
+#pragma omp teams reduction(+: a[0:2])
+ { }
+
+#pragma omp target map(vla[0:2])
+#pragma omp teams reduction(+: vla[0:2])
+ { }
+
+#ifdef NO_VLA
+ // expected-error@+4 {{cannot generate code for reduction on array section, which requires a variable-length array}}
+ // expected-note@+3 {{the target device does not support allocating variable-length arrays}}
+#endif
+#pragma omp target map(a[0:arg])
+#pragma omp teams reduction(+: a[0:arg])
+ { }
+
+#ifdef NO_VLA
+ // expected-error@+4 {{cannot generate code for reduction on array section, which requires a variable-length array}}
+ // expected-note@+3 {{the target device does not support allocating variable-length arrays}}
+#endif
+#pragma omp target map(vla[0:arg])
+#pragma omp teams reduction(+: vla[0:arg])
+ { }
+}
+
+void parallel_reduction(int arg) {
+ int a[2];
+ int vla[arg];
+
+#pragma omp target map(a)
+#pragma omp parallel reduction(+: a)
+ { }
+
+#ifdef NO_VLA
+ // expected-error@+4 {{cannot generate code for reduction on variable-length array}}
+ // expected-note@+3 {{the target device does not support allocating variable-length arrays}}
+#endif
+#pragma omp target map(vla)
+#pragma omp parallel reduction(+: vla)
+ { }
+
+#pragma omp target map(a[0:2])
+#pragma omp parallel reduction(+: a[0:2])
+ { }
+
+#pragma omp target
