[OpenMP] Do not propagate match extensions to nested contexts

[jdoerfert]

Summary

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

[tianshilei1992]

So I suppose D95765 can replace this patch, right?

[jdoerfert]

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.

[tianshilei1992]

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?

[jdoerfert]

Update docs

[jdoerfert]

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.

[JonChesterfield]

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.

[jdoerfert]

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.

[JonChesterfield]

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

clang/include/clang/Basic/AttrDocs.td

.. code-block:: none
     match_none
     disable_implicit_base
     allow_templates
 
 The match extensions change when the *entire* context selector is considered a
 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
 found. The variant is still generated but will never be called, due to the
 absence of a base function and consequently calls to a base function.
 The allow extensions change when the ``begin declare variant`` effect is
 applied to a definition. If ``allow_templates`` is given, template function
 definitions are considered as specializations of existing or assumed template

clang/lib/Parse/ParseOpenMP.cpp

 #include "clang/Basic/OpenMPKinds.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Parse/ParseDiagnostic.h"
 #include "clang/Parse/Parser.h"
 #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"
 
 using namespace clang;
 using namespace llvm::omp;
 
 //===----------------------------------------------------------------------===//
bool Parser::parseOMPDeclareVariantMatchClause(SourceLocation Loc,
   if (!ParentTI)
     return false;
 
   // Merge the parent/outer trait info into the one we just parsed and diagnose
   // problems.
   // 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 &ParentSet : ParentTI->Sets) {
+  auto StripImplementation = [](const OMPTraitSet &TSet) -> OMPTraitSet {
+    if (TSet.Kind != llvm::omp::TraitSet::implementation)
+      return TSet;
+    OMPTraitSet Set = TSet;
+    for (OMPTraitSelector &Selector : 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 &Property) {
+        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 &PSet : ParentTI->Sets) {
+    const OMPTraitSet ParentSet = StripImplementation(PSet);
     bool MergedSet = false;
     for (OMPTraitSet &Set : TI.Sets) {
       if (Set.Kind != ParentSet.Kind)
         continue;
       MergedSet = true;
       for (const OMPTraitSelector &ParentSelector : ParentSet.Selectors) {
         bool MergedSelector = false;
         for (OMPTraitSelector &Selector : Set.Selectors) {

clang/test/OpenMP/begin_declare_variant_nested_propagation.c

+// 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

llvm/lib/Frontend/OpenMP/OMPContext.cpp

static int isVariantApplicableInContextHelper(
   // Helper to deal with a single property that was (not) found in the OpenMP
   // context based on the match kind selected by the user via
   // `implementation={extensions(match_[all,any,none])}'
   auto HandleTrait = [MK](TraitProperty Property,
                           bool WasFound) -> Optional<bool> /* Result */ {
     // 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;
     }
 
     // In "all" or "none" mode we accept a matching or non-matching property
     // respectively and move on. We are not done yet!
     if ((WasFound && MK == MK_ALL) || (!WasFound && MK == MK_NONE))
       return None;