[flang][OpenMP] Fix common block missing symbol crash

[NimishMishra]

Fixes #65034 by skipping copy of host-association information if the concerned symbol is missing from the inner construct

[llvmbot]

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

@llvm/pr-subscribers-flang-openmp

Changes

Fixes #65034 by skipping copy of host-association information if the concerned symbol is missing from the inner construct

---

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

4 Files Affected:

• (modified) flang/include/flang/Lower/AbstractConverter.h (+4)
• (modified) flang/lib/Lower/Bridge.cpp (+7)
• (modified) flang/lib/Lower/OpenMP.cpp (+2-1)
• (modified) flang/test/Lower/OpenMP/FIR/copyin.f90 (+40)

diff --git a/flang/include/flang/Lower/AbstractConverter.h b/flang/include/flang/Lower/AbstractConverter.h
index 477c8164a18eafd..842dd7adf247d05 100644
--- a/flang/include/flang/Lower/AbstractConverter.h
+++ b/flang/include/flang/Lower/AbstractConverter.h
@@ -108,6 +108,10 @@ class AbstractConverter {
       const Fortran::semantics::Symbol &sym,
       mlir::OpBuilder::InsertPoint *copyAssignIP = nullptr) = 0;
 
+  /// For a given symbol, check if it is present in the inner-most
+  /// level of the symbol map.
+  virtual bool isPresentShallowLookup(Fortran::semantics::Symbol &sym) = 0;
+
   /// Collect the set of symbols with \p flag in \p eval
   /// region if \p collectSymbols is true. Likewise, collect the
   /// set of the host symbols with \p flag of the associated symbols in \p eval
diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp
index 2fb3c2c3818d42d..44d3b186093c651 100644
--- a/flang/lib/Lower/Bridge.cpp
+++ b/flang/lib/Lower/Bridge.cpp
@@ -588,6 +588,13 @@ class FirConverter : public Fortran::lower::AbstractConverter {
         std::nullopt);
   }
 
+  bool isPresentShallowLookup(Fortran::semantics::Symbol &sym) override final {
+    Fortran::lower::SymbolBox sb = shallowLookupSymbol(sym);
+    if (shallowLookupSymbol(sym))
+      return true;
+    return false;
+  }
+
   bool createHostAssociateVarClone(
       const Fortran::semantics::Symbol &sym) override final {
     mlir::Location loc = genLocation(sym.name());
diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp
index 5f5e968eaaa6414..1b81336831361c7 100644
--- a/flang/lib/Lower/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP.cpp
@@ -1551,7 +1551,8 @@ bool ClauseProcessor::processCopyin() const {
           mlir::OpBuilder::InsertPoint *copyAssignIP = nullptr) {
         assert(sym->has<Fortran::semantics::HostAssocDetails>() &&
                "No host-association found");
-        converter.copyHostAssociateVar(*sym, copyAssignIP);
+        if (converter.isPresentShallowLookup(*sym))
+          converter.copyHostAssociateVar(*sym, copyAssignIP);
       };
   bool hasCopyin = findRepeatableClause<ClauseTy::Copyin>(
       [&](const ClauseTy::Copyin *copyinClause,
diff --git a/flang/test/Lower/OpenMP/FIR/copyin.f90 b/flang/test/Lower/OpenMP/FIR/copyin.f90
index ddfa0ea0914628f..e1dcd8f7402afb8 100644
--- a/flang/test/Lower/OpenMP/FIR/copyin.f90
+++ b/flang/test/Lower/OpenMP/FIR/copyin.f90
@@ -311,3 +311,43 @@ subroutine common_2()
      end do
   !$omp end parallel do
 end subroutine
+
+!CHECK: func.func @_QPcommon_3() {
+!CHECK: %[[val_0:.*]] = fir.address_of(@blk_) : !fir.ref<!fir.array<8xi8>>
+!CHECK: %[[val_1:.*]] = omp.threadprivate %[[val_0]] : !fir.ref<!fir.array<8xi8>> -> !fir.ref<!fir.array<8xi8>>
+!CHECK: %[[val_2:.*]] = fir.convert %[[val_1]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK: %[[val_c4:.*]] = arith.constant 4 : index
+!CHECK: %[[val_3:.*]] = fir.coordinate_of %[[val_2]], %[[val_c4]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK: %[[val_4:.*]] = fir.convert %[[val_3]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK: omp.parallel {
+!CHECK: %[[val_5:.*]] = omp.threadprivate %[[val_0]] : !fir.ref<!fir.array<8xi8>> -> !fir.ref<!fir.array<8xi8>>
+!CHECK: %[[val_6:.*]] = fir.convert %[[val_5]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK: %[[val_c4_0:.*]] = arith.constant 4 : index
+!CHECK: %[[val_7:.*]] = fir.coordinate_of %[[val_6]], %[[val_c4_0]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK: %[[val_8:.*]] = fir.convert %[[val_7]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK: %[[val_9:.*]] = fir.load %[[val_4]] : !fir.ref<i32>
+!CHECK: fir.store %[[val_9]] to %[[val_8]] : !fir.ref<i32>
+!CHECK: omp.barrier
+!CHECK: omp.sections {
+!CHECK: omp.section {
+!CHECK: %[[val_10:.*]] = fir.load %[[val_8]] : !fir.ref<i32>
+!CHECK: %[[val_c3_i32:.*]] = arith.constant 3 : i32
+!CHECK: %[[val_11:.*]] = arith.addi %[[val_10]], %[[val_c3_i32]] : i32
+!CHECK: fir.store %[[val_11]] to %[[val_8]] : !fir.ref<i32>
+!CHECK: omp.terminator
+!CHECK: }
+!CHECK: omp.terminator
+!CHECK: }
+!CHECK: return
+!CHECK: }
+subroutine common_3()
+  integer :: x
+  integer :: y
+  common /blk/ x, y
+  !$omp threadprivate (/blk/)
+
+  !$omp parallel sections copyin(/blk/)
+        !$omp section
+            y = y + 3
+  !$omp end parallel sections
+end subroutine

[NimishMishra]

The copyHostAssociateVar function captures two symbol boxes: one from lookupOneLevelUpSymbol which fetches the original copy of the variable, and one from shallowLookupSymbol which fetches the copy that intends to mask the original variable.

However, in case of common block, because of the abstraction involved, there is a chance that shallowLookupSymbol fails to find a symbol box because relevant symbol mapping has not been created. For instance,

  program sample
  integer:: x
  integer :: y
  common /blk/ x
  !$omp threadprivate (/blk/)
  !$omp parallel sections copyin(/blk/)
    !$omp section
      y = y + 1
  !$omp end parallel sections
 end program sample

Dumping the parse-tree for parallel sections reveals that x is not explicitly used in the construct, thereby it being missing from the symbol map of the construct. Now when copyin executes, it fails to find x in the inner construct's symbol map, thereby erring out a LLVM ERROR: symbol not mapped. This PR fixes the same. It only executes the relevant copyin functionality iff it is sure that the symbol exists in the symbol map of the inner construct (i.e. the relevant variable is used in the inner construct).

[NimishMishra]

Ping for review!

[shraiysh]

Why is this called twice? And shouldn't this function just be

bool isPresentShallowLookup(Fortran::semantics::Symbol &sym) {
  return shallowLookupSymbol(sym)
}

And in that case do we need to make this function, or can we directly call this shallowLookup() function at the use site?

[NimishMishra]

shallowLookupSymbol() does not seem to be exposed to any interface we have access to at the call site. So I added this interface in AbstractConverter. I'll recheck once though.

And will modify isPresentShallowLookup is at all it needs to be there.

[NimishMishra]

@shraiysh I checked this. shallowLookupSymbol is operating upon a symbol map localSymbols defined and maintained in Bridge.cpp. So in any case, we need to allow an interface to access the same during lowering OpenMP clauses.

[shraiysh]

LG.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.