[clang] [flang] [llvm] [clang][flang][OpenMP] Fix context selector matching and scoring (PR #224431)

[via cfe-commits]

================
@@ -1473,27 +1490,90 @@ semantics::omp::OmpVariantMatchContext makeVariantMatchContext(
 }
 
 void collectEnclosingConstructTraits(
-    mlir::Operation *op,
+    AbstractConverter &converter, const pft::Evaluation *evaluation,
     llvm::SmallVectorImpl<llvm::omp::TraitProperty> &constructTraits) {
-  // Collect enclosing OpenMP operations so variants chosen by an outer
-  // metadirective are part of this metadirective's context. For example, an
-  // inner metadirective inside `target` and an outer-selected `parallel` must
-  // be able to match construct={target, parallel}. The final reverse yields
-  // outermost-to-innermost order as required by OMPContext.
-  for (; op; op = op->getParentOp()) {
-    if (mlir::isa<mlir::omp::WsloopOp>(op))
-      constructTraits.push_back(llvm::omp::TraitProperty::construct_for_for);
-    if (mlir::isa<mlir::omp::ParallelOp>(op))
-      constructTraits.push_back(
-          llvm::omp::TraitProperty::construct_parallel_parallel);
-    if (mlir::isa<mlir::omp::TeamsOp>(op))
-      constructTraits.push_back(
-          llvm::omp::TraitProperty::construct_teams_teams);
-    if (mlir::isa<mlir::omp::TargetOp>(op))
-      constructTraits.push_back(
-          llvm::omp::TraitProperty::construct_target_target);
+  const auto *loopControl =
+      converter.getStateStack().getStackTop<LoopControlContext>();
+  // Use the loop owner's ancestors: host evaluation may have TARGET current
+  // while evaluating a nested loop's bounds.
+  if (loopControl)
+    evaluation = &loopControl->evaluation;
+
+  llvm::SmallVector<const OpenMPContextFrame *, 4> frames;
+  converter.getStateStack().stackWalk<OpenMPContextFrame>(
+      [&](OpenMPContextFrame &frame) {
+        frames.push_back(&frame);
+        return mlir::WalkResult::advance();
+      });
+  std::reverse(frames.begin(), frames.end());
+  llvm::SmallVector<bool, 4> usedFrames(frames.size(), false);
+
+  llvm::SmallVector<const pft::Evaluation *, 8> ancestors;
+  for (const pft::Evaluation *parent = evaluation ? evaluation->parentConstruct
+                                                  : nullptr;
+       parent; parent = parent->parentConstruct) {
+    ancestors.push_back(parent);
+  }
+  std::reverse(ancestors.begin(), ancestors.end());
+
+  auto append = [&](llvm::omp::Directive directive) {
+    semantics::omp::AppendDirectiveContextTraits(directive, constructTraits);
+  };
+  for (const pft::Evaluation *ancestor : ancestors) {
+    const auto *omp = ancestor->getIf<parser::OpenMPConstruct>();
+    if (!omp)
+      continue;
+    llvm::omp::Directive directive{parser::omp::GetOmpDirectiveName(*omp).v};
+    // An ancestor supplies the full source context, including constituents
+    // whose bodies also have active frames. Count each construct only once.
+    for (auto [index, frame] : llvm::enumerate(frames))
+      if (&frame->evaluation == ancestor)
+        usedFrames[index] = true;
+    if (directive != llvm::omp::Directive::OMPD_metadirective) {
+      append(directive);
+      continue;
+    }
+    for (const OpenMPContextFrame *frame : frames) {
+      if (&frame->evaluation == ancestor && frame->isReplacement) {
+        append(frame->directive);
+        break;
+      }
+    }
+  }
+
+  // Use entered frames for clauses. Loop bounds use the source prefix below,
+  // which is available even before the owner's frames are entered.
+  for (auto [index, frame] : llvm::enumerate(frames)) {
+    if (usedFrames[index] || frame->isReplacement)
+      continue;
+    if (loopControl && &frame->evaluation == evaluation)
+      continue;
+    append(frame->directive);
+  }
+
+  if (!loopControl)
+    return;
+  const auto *omp = evaluation->getIf<parser::OpenMPConstruct>();
+  if (!omp)
+    return;
+  llvm::omp::Directive directive = parser::omp::GetOmpDirectiveName(*omp).v;
+  if (directive == llvm::omp::Directive::OMPD_metadirective) {
+    for (const OpenMPContextFrame *frame : frames) {
+      if (&frame->evaluation == evaluation && frame->isReplacement) {
+        directive = frame->directive;
+        break;
+      }
+    }
+  }
+  // Bounds precede the first loop-associated constituent in source order.
+  // TARGET TEAMS DISTRIBUTE PARALLEL DO therefore retains TARGET and TEAMS.
+  for (llvm::omp::Directive leaf :
+       llvm::omp::getLeafConstructsOrSelf(directive)) {
+    if (llvm::omp::getDirectiveAssociation(leaf) ==
+        llvm::omp::Association::LoopNest)
----------------
MattPD wrote:

FUSE is associated with a loop sequence rather than a loop nest, so the walk does not stop at FUSE and appends it to the construct context. `genCanonicalLoopNest` first evaluates each bound through `collectLoopRelatedInfo` and discards the result. It then evaluates the bound again to compute the trip count. Only the first evaluation has FUSE in its context. The `arch` selector's weight grows with the number of constructs in the context, so the extra FUSE construct can change the variant selected in the first evaluation.

At 9d79b83 on an x86_64 host, the HLFIR from `flang -fc1 -fopenmp -fopenmp-version=60 -emit-hlfir` for the following module calls `v_arch` in the first evaluation of each upper bound and `v_vendor` in its trip-count evaluation. At 89a879d, both evaluations call `v_vendor`. For a single loop under `tile sizes(2)`, both evaluations still call `v_vendor` at 9d79b83.

```fortran
module m
contains
  integer function v_vendor(n)
    integer, intent(in) :: n
    v_vendor = n
  end function
  integer function v_arch(n)
    integer, intent(in) :: n
    v_arch = n
  end function
  integer function value(n)
    integer, intent(in) :: n
    !$omp declare variant(v_vendor) match(implementation={vendor(score(3): llvm)})
    !$omp declare variant(v_arch) match(device={arch(x86_64)})
    value = n
  end function
  subroutine s(n, a, b)
    integer :: n, a(100), b(100), i, j
    !$omp fuse
    do i = 1, value(n)
      a(i) = i
    end do
    do j = 1, value(n)
      b(j) = j
    end do
    !$omp end fuse
  end subroutine
end module
```

The trip count still comes from `v_vendor`. The first evaluation's call still runs, so any side effects of that call now come from `v_arch` instead of `v_vendor`.

Should the walk also stop at `Association::LoopSequence`?

https://github.com/llvm/llvm-project/pull/224431