[flang-commits] [flang] [flang][OpenMP] Fix crash on standalone ordered with depend(source|sink:) (PR #200193)

[via flang-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-flang-fir-hlfir

Author: Carlos Seo (ceseo)

<details>
<summary>Changes</summary>

A standalone ordered construct using the pre-OpenMP 5.2 depend(source) / depend(sink:) spelling crashed flang with an assertion failure in buildConstructQueue ("Construct decomposition failed"), or emitted a TODO when assertions were disabled.

These dependence types are valid on ordered since OpenMP 4.5, but flang represents them internally as a doacross clause, which construct decomposition only accepts from OpenMP 5.2. As a result, decomposition produced an empty output and tripped the assertion at every OpenMP version below 5.2 (including the default 3.1).

Lowering of the standalone ordered directive is not implemented yet (genOrderedOp only emits a "not yet implemented" message and ignores the construct queue). Build the construct queue only for the block-associated variant and emit the TODO directly for the standalone directive, so the decomposition that would otherwise assert is no longer reached.

Note: at versions below 4.5 the construct is technically invalid, but flang's semantics accepts it silently. Making semantics emit a "requires OpenMP 4.5" error/warning is probably a good idea.

Fixes #<!-- -->198972

---
Full diff: https://github.com/llvm/llvm-project/pull/200193.diff


2 Files Affected:

- (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+17-13) 
- (added) flang/test/Lower/OpenMP/Todo/ordered-depend.f90 (+20) 


``````````diff
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 266b06f353675..aa21fbea0e5e3 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -2585,11 +2585,11 @@ genMasterOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
       queue, item);
 }
 
-static mlir::omp::OrderedOp
-genOrderedOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
-             semantics::SemanticsContext &semaCtx, lower::pft::Evaluation &eval,
-             mlir::Location loc, const ConstructQueue &queue,
-             ConstructQueue::const_iterator item) {
+static mlir::omp::OrderedOp genOrderedOp(lower::AbstractConverter &converter,
+                                         lower::SymMap &symTable,
+                                         semantics::SemanticsContext &semaCtx,
+                                         lower::pft::Evaluation &eval,
+                                         mlir::Location loc) {
   if (!semaCtx.langOptions().OpenMPSimd)
     TODO(loc, "OMPD_ordered");
   return nullptr;
@@ -4666,18 +4666,22 @@ static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
   List<Clause> clauses = makeClauses(construct.v.Clauses(), semaCtx);
   mlir::Location currentLocation = converter.genLocation(directive.source);
 
+  if (directive.v == llvm::omp::Directive::OMPD_ordered) {
+    // Standalone "ordered" directive. Lowering is not yet implemented, so emit
+    // the "not yet implemented" message directly rather than going through
+    // construct decomposition. The latter would assert for the (pre-5.2)
+    // "depend(source|sink:)" spelling, which is represented internally as a
+    // "doacross" clause that decomposition only accepts from OpenMP 5.2.
+    genOrderedOp(converter, symTable, semaCtx, eval, currentLocation);
+    return;
+  }
+
+  // Dispatch handles the "block-associated" variant of "ordered".
   ConstructQueue queue{
       buildConstructQueue(converter.getFirOpBuilder().getModule(), semaCtx,
                           eval, directive.source, directive.v, clauses)};
-  if (directive.v == llvm::omp::Directive::OMPD_ordered) {
-    // Standalone "ordered" directive.
-    genOrderedOp(converter, symTable, semaCtx, eval, currentLocation, queue,
+  genOMPDispatch(converter, symTable, semaCtx, eval, currentLocation, queue,
                  queue.begin());
-  } else {
-    // Dispatch handles the "block-associated" variant of "ordered".
-    genOMPDispatch(converter, symTable, semaCtx, eval, currentLocation, queue,
-                   queue.begin());
-  }
 }
 
 static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
diff --git a/flang/test/Lower/OpenMP/Todo/ordered-depend.f90 b/flang/test/Lower/OpenMP/Todo/ordered-depend.f90
new file mode 100644
index 0000000000000..f7623c23ad7e3
--- /dev/null
+++ b/flang/test/Lower/OpenMP/Todo/ordered-depend.f90
@@ -0,0 +1,20 @@
+!RUN: %not_todo_cmd %flang_fc1 -emit-hlfir -fopenmp -o - %s 2>&1 | FileCheck %s
+
+! Issue #198972: a standalone "ordered" construct using the (pre-5.2)
+! "depend(source)" / "depend(sink:)" spelling must reach the "not yet
+! implemented" path instead of crashing during construct decomposition. The
+! depend(source|sink:) spelling is represented internally as a "doacross"
+! clause, which decomposition only accepts from OpenMP 5.2, while the construct
+! itself is valid (using this spelling) since OpenMP 4.5.
+
+!CHECK: not yet implemented: OMPD_ordered
+subroutine f00
+  integer :: i
+  !$omp do ordered(1)
+  do i = 1, 10
+    !$omp ordered depend(source)
+    !$omp ordered depend(sink: i - 1)
+  end do
+  !$omp end do
+end
+

``````````

</details>


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