[PATCH] D95764: [OpenMP] Do not propagate match extensions to nested contexts

[Johannes Doerfert via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGad9e98b8efa0: [OpenMP] Do not propagate match extensions to 
nested contexts (authored by jdoerfert).
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D95764?vs=320371=330141#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95764

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/Parse/ParseOpenMP.cpp
  clang/test/OpenMP/begin_declare_variant_nested_propagation.c
  llvm/lib/Frontend/OpenMP/OMPContext.cpp


Index: llvm/lib/Frontend/OpenMP/OMPContext.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPContext.cpp
+++ llvm/lib/Frontend/OpenMP/OMPContext.cpp
@@ -168,8 +168,13 @@
 // For kind "any" a single match is enough but we ignore non-matched
 // properties.
 if (MK == MK_ANY) {
-  if (WasFound)
+  if (WasFound) {
+LLVM_DEBUG(
+dbgs() << "[" << DEBUG_TYPE << "] Property "
+   << getOpenMPContextTraitPropertyName(Property, "")
+   << " was in the OpenMP context and match kind is any.\n";);
 return true;
+  }
   return None;
 }
 
Index: clang/test/OpenMP/begin_declare_variant_nested_propagation.c
===
--- /dev/null
+++ clang/test/OpenMP/begin_declare_variant_nested_propagation.c
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -verify -fopenmp -x c -std=c99 
-fms-extensions -Wno-pragma-pack %s
+// expected-no-diagnostics
+
+#pragma omp begin declare variant match(user={condition(1)}, 
device={kind(cpu)}, implementation={extension(match_any)})
+#pragma omp begin declare variant match(device = {kind(cpu, fpga)})
+ this is never reached
+#pragma omp end declare variant
+#pragma omp end declare variant
+
+#pragma omp begin declare variant match(user={condition(1)}, 
device={kind(cpu)}, implementation={extension(match_any)})
+#pragma omp begin declare variant match(device = {kind(cpu, fpga)}, 
implementation={vendor(llvm)})
+ this is never reached
+#pragma omp end declare variant
+#pragma omp end declare variant
Index: clang/lib/Parse/ParseOpenMP.cpp
===
--- clang/lib/Parse/ParseOpenMP.cpp
+++ clang/lib/Parse/ParseOpenMP.cpp
@@ -21,6 +21,7 @@
 #include "clang/Parse/RAIIObjectsForParser.h"
 #include "clang/Sema/Scope.h"
 #include "llvm/ADT/PointerIntPair.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/UniqueVector.h"
 #include "llvm/Frontend/OpenMP/OMPContext.h"
@@ -1463,7 +1464,29 @@
   // TODO: Keep some source location in the TI to provide better diagnostics.
   // TODO: Perform some kind of equivalence check on the condition and score
   //   expressions.
-  for (const OMPTraitSet  : ParentTI->Sets) {
+  auto StripImplementation = [](const OMPTraitSet ) -> OMPTraitSet {
+if (TSet.Kind != llvm::omp::TraitSet::implementation)
+  return TSet;
+OMPTraitSet Set = TSet;
+for (OMPTraitSelector  : Set.Selectors) {
+  if (Selector.Kind != llvm::omp::TraitSelector::implementation_extension)
+continue;
+  // Do not propagate match extensions to nested contexts.
+  llvm::erase_if(Selector.Properties, [](const OMPTraitProperty ) 
{
+return (
+Property.Kind ==
+llvm::omp::TraitProperty::implementation_extension_match_any ||
+Property.Kind ==
+llvm::omp::TraitProperty::implementation_extension_match_all ||
+Property.Kind ==
+llvm::omp::TraitProperty::implementation_extension_match_none);
+  });
+  return Set;
+}
+return Set;
+  };
+  for (const OMPTraitSet  : ParentTI->Sets) {
+const OMPTraitSet ParentSet = StripImplementation(PSet);
 bool MergedSet = false;
 for (OMPTraitSet  : TI.Sets) {
   if (Set.Kind != ParentSet.Kind)
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -4014,7 +4014,8 @@
 match for an OpenMP context. The default is ``all``, with ``none`` no trait in 
the
 selector is allowed to be in the OpenMP context, with ``any`` a single trait in
 both the selector and OpenMP context is sufficient. Only a single match
-extension trait is allowed per context selector.
+extension trait is allowed per context selector. Note that match extensions are
+not propagated to nested selectors like the standard defines it for other 
traits.
 The disable extensions remove default effects of the ``begin declare variant``
 applied to a definition. If ``disable_implicit_base`` 

[PATCH] D95764: [OpenMP] Do not propagate match extensions to nested contexts

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

That sounds reasonable. We can probably expect features to be renamed and 
semantically adjusted on their way to standardisation anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95764

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

[PATCH] D95764: [OpenMP] Do not propagate match extensions to nested contexts

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D95764#2532927 , @JonChesterfield 
wrote:

> I would have expected a nested match statement to hit on a subset of those 
> that matched in the parent.
>
> If (match x64)
> If (match Intel)
>
> - expect x64 and intel to be true here
>
> I think that's how the normal openmp variant match works.

It is.

> Why do we want to diverge from that for (all?) extensions?

For the same reason we have `match_any` and other things that modify the 
standard behavior, it is useful or even necessary to solve certain problems.

> It would make for a semantic break if one of the extensions was standardised.

Yes and no. Like a breaking change in the standard, that could happen on a 
conceptual level. However, if the standard would adopt part of the `match_any`
semantics, it would not assume it to be in the `extension` trait set. Doing so 
would introduce problems for downstream extensions which are basically
breaking changes. That said, you have to remember that the semantic of 
`match_any` (and `match_none`) was made up for LLVM and is already changing the
behavior of the variant selector compared to the default described in the 
standard. The argument of this patch is that this made up semantics should be
tweaked. FWIW, when I added the extension I haven't had implemented the 
selector inheritance yet, so at that point there was no practical difference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95764

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

[PATCH] D95764: [OpenMP] Do not propagate match extensions to nested contexts

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

I would have expected a nested match statement to hit on a subset of those that 
matched in the parent.

If (match x64)
If (match Intel)

- expect x64 and intel to be true here

I think that's how the normal openmp variant match works. Why do we want to 
diverge from that for (all?) extensions? It would make for a semantic break if 
one of the extensions was standardised.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95764

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

[PATCH] D95764: [OpenMP] Do not propagate match extensions to nested contexts

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D95764#2532908 , @tianshilei1992 
wrote:

> In D95764#2532877 , @jdoerfert wrote:
>
>> In D95764#2532875 , @tianshilei1992 
>> wrote:
>>
>>> So I suppose D95765  can replace this 
>>> patch, right?
>>
>> I think we want both. Propagating match clauses doesn't make sense to me so 
>> we should never do it.
>
> Is there any place documenting the propagation?

The standard documents that nested contexts will inherit the selectors, in a 
specific way. given that this is an llvm extension, we can overwrite that 
though. I added the documentation changes now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95764

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

[PATCH] D95764: [OpenMP] Do not propagate match extensions to nested contexts

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 320371.
jdoerfert added a comment.
Herald added a reviewer: aaron.ballman.

Update docs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95764

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/Parse/ParseOpenMP.cpp
  clang/test/OpenMP/begin_declare_variant_nested_propagation.c


Index: clang/test/OpenMP/begin_declare_variant_nested_propagation.c
===
--- /dev/null
+++ clang/test/OpenMP/begin_declare_variant_nested_propagation.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -verify -fopenmp -x c -std=c99 
-fms-extensions -Wno-pragma-pack %s
+// expected-no-diagnostics
+
+#pragma omp begin declare variant match(implementation={extension(match_any)})
+#pragma omp begin declare variant match(device = {vendor(cray, ibm)})
+ this is never reached, we cannot have a cray ibm compiler hybrid, I hope.
+#pragma omp end declare variant
+#pragma omp end declare variant
Index: clang/lib/Parse/ParseOpenMP.cpp
===
--- clang/lib/Parse/ParseOpenMP.cpp
+++ clang/lib/Parse/ParseOpenMP.cpp
@@ -1477,6 +1477,14 @@
   MergedSelector = true;
   for (const OMPTraitProperty  :
ParentSelector.Properties) {
+// Do not propagate match extensions to nested contexts.
+if (ParentProperty.Kind == llvm::omp::TraitProperty::
+   implementation_extension_match_any 
||
+ParentProperty.Kind == llvm::omp::TraitProperty::
+   implementation_extension_match_all 
||
+ParentProperty.Kind == llvm::omp::TraitProperty::
+   implementation_extension_match_none)
+  continue;
 bool MergedProperty = false;
 for (OMPTraitProperty  : Selector.Properties) {
   // Ignore "equivalent" properties.
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -3965,7 +3965,8 @@
 match for an OpenMP context. The default is ``all``, with ``none`` no trait in 
the
 selector is allowed to be in the OpenMP context, with ``any`` a single trait in
 both the selector and OpenMP context is sufficient. Only a single match
-extension trait is allowed per context selector.
+extension trait is allowed per context selector. Note that match extensions are
+not propagated to nested selectors like the standard defines it for other 
traits.
 The disable extensions remove default effects of the ``begin declare variant``
 applied to a definition. If ``disable_implicit_base`` is given, we will not
 introduce an implicit base function for a variant if no base function was


Index: clang/test/OpenMP/begin_declare_variant_nested_propagation.c
===
--- /dev/null
+++ clang/test/OpenMP/begin_declare_variant_nested_propagation.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -verify -fopenmp -x c -std=c99 -fms-extensions -Wno-pragma-pack %s
+// expected-no-diagnostics
+
+#pragma omp begin declare variant match(implementation={extension(match_any)})
+#pragma omp begin declare variant match(device = {vendor(cray, ibm)})
+ this is never reached, we cannot have a cray ibm compiler hybrid, I hope.
+#pragma omp end declare variant
+#pragma omp end declare variant
Index: clang/lib/Parse/ParseOpenMP.cpp
===
--- clang/lib/Parse/ParseOpenMP.cpp
+++ clang/lib/Parse/ParseOpenMP.cpp
@@ -1477,6 +1477,14 @@
   MergedSelector = true;
   for (const OMPTraitProperty  :
ParentSelector.Properties) {
+// Do not propagate match extensions to nested contexts.
+if (ParentProperty.Kind == llvm::omp::TraitProperty::
+   implementation_extension_match_any ||
+ParentProperty.Kind == llvm::omp::TraitProperty::
+   implementation_extension_match_all ||
+ParentProperty.Kind == llvm::omp::TraitProperty::
+   implementation_extension_match_none)
+  continue;
 bool MergedProperty = false;
 for (OMPTraitProperty  : Selector.Properties) {
   // Ignore "equivalent" properties.
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -3965,7 +3965,8 @@
 match for an OpenMP context. The default is ``all``, with ``none`` no trait in the
 

[PATCH] D95764: [OpenMP] Do not propagate match extensions to nested contexts

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 added a comment.

In D95764#2532877 , @jdoerfert wrote:

> In D95764#2532875 , @tianshilei1992 
> wrote:
>
>> So I suppose D95765  can replace this 
>> patch, right?
>
> I think we want both. Propagating match clauses doesn't make sense to me so 
> we should never do it.

Is there any place documenting the propagation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95764

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

[PATCH] D95764: [OpenMP] Do not propagate match extensions to nested contexts

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D95764#2532875 , @tianshilei1992 
wrote:

> So I suppose D95765  can replace this patch, 
> right?

I think we want both. Propagating match clauses doesn't make sense to me so we 
should never do it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95764

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

[PATCH] D95764: [OpenMP] Do not propagate match extensions to nested contexts

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 added a comment.

So I suppose D95765  can replace this patch, 
right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95764

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

[PATCH] D95764: [OpenMP] Do not propagate match extensions to nested contexts

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert created this revision.
jdoerfert added reviewers: JonChesterfield, tianshilei1992.
Herald added subscribers: guansong, yaxunl.
Herald added a reviewer: bollu.
jdoerfert requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

If we have nested declare variant context, it doesn't make sense to
inherit the match extension from the parent. Instead, just skip it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95764

Files:
  clang/lib/Parse/ParseOpenMP.cpp
  clang/test/OpenMP/begin_declare_variant_nested_propagation.c


Index: clang/test/OpenMP/begin_declare_variant_nested_propagation.c
===
--- /dev/null
+++ clang/test/OpenMP/begin_declare_variant_nested_propagation.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -verify -fopenmp -x c -std=c99 
-fms-extensions -Wno-pragma-pack %s
+// expected-no-diagnostics
+
+#pragma omp begin declare variant match(implementation={extension(match_any)})
+#pragma omp begin declare variant match(device = {vendor(cray, ibm)})
+ this is never reached, we cannot have a cray ibm compiler hybrid, I hope.
+#pragma omp end declare variant
+#pragma omp end declare variant
Index: clang/lib/Parse/ParseOpenMP.cpp
===
--- clang/lib/Parse/ParseOpenMP.cpp
+++ clang/lib/Parse/ParseOpenMP.cpp
@@ -1477,6 +1477,14 @@
   MergedSelector = true;
   for (const OMPTraitProperty  :
ParentSelector.Properties) {
+// Do not propagate match extensions to nested contexts.
+if (ParentProperty.Kind == llvm::omp::TraitProperty::
+   implementation_extension_match_any 
||
+ParentProperty.Kind == llvm::omp::TraitProperty::
+   implementation_extension_match_all 
||
+ParentProperty.Kind == llvm::omp::TraitProperty::
+   implementation_extension_match_none)
+  continue;
 bool MergedProperty = false;
 for (OMPTraitProperty  : Selector.Properties) {
   // Ignore "equivalent" properties.


Index: clang/test/OpenMP/begin_declare_variant_nested_propagation.c
===
--- /dev/null
+++ clang/test/OpenMP/begin_declare_variant_nested_propagation.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -verify -fopenmp -x c -std=c99 -fms-extensions -Wno-pragma-pack %s
+// expected-no-diagnostics
+
+#pragma omp begin declare variant match(implementation={extension(match_any)})
+#pragma omp begin declare variant match(device = {vendor(cray, ibm)})
+ this is never reached, we cannot have a cray ibm compiler hybrid, I hope.
+#pragma omp end declare variant
+#pragma omp end declare variant
Index: clang/lib/Parse/ParseOpenMP.cpp
===
--- clang/lib/Parse/ParseOpenMP.cpp
+++ clang/lib/Parse/ParseOpenMP.cpp
@@ -1477,6 +1477,14 @@
   MergedSelector = true;
   for (const OMPTraitProperty  :
ParentSelector.Properties) {
+// Do not propagate match extensions to nested contexts.
+if (ParentProperty.Kind == llvm::omp::TraitProperty::
+   implementation_extension_match_any ||
+ParentProperty.Kind == llvm::omp::TraitProperty::
+   implementation_extension_match_all ||
+ParentProperty.Kind == llvm::omp::TraitProperty::
+   implementation_extension_match_none)
+  continue;
 bool MergedProperty = false;
 for (OMPTraitProperty  : Selector.Properties) {
   // Ignore "equivalent" properties.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits