[flang-commits] [flang] [Flang][OpenMP] Fix Common Blocks use in update to/from and target maps causing compiler errors (PR #187221)
via flang-commits
flang-commits at lists.llvm.org
Wed Mar 18 02:23:18 PDT 2026
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-flang-openmp
Author: None (agozillon)
<details>
<summary>Changes</summary>
This patch attempts to fix a compiler ICE when common blocks are used in target update to/from, it seems to stem from the fact that we do not resolve the symbols in the relevant clauses, so when we later process the maps we don't have the right symbol that references the common block that was setup and bound by the fortran lowering. Resolving the names seems to do the trick.
There is a second issue where when referencing a common block with an array contained in it and utilising the array within the target region, we'll currently not accurately map over the bounds and cause a FIR/MLIR verification error. The fix for this is to simply move the common block member re-binding/re-materialization for the target region to before the bounds data re-materialization we do during target region generation.
---
Full diff: https://github.com/llvm/llvm-project/pull/187221.diff
3 Files Affected:
- (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+24-10)
- (modified) flang/lib/Semantics/resolve-directives.cpp (+42-1)
- (added) flang/test/Lower/OpenMP/common-block-target-update.f90 (+30)
``````````diff
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index ae5f6f50bda09..1efd4ee6489e4 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -980,6 +980,20 @@ getImplicitMapTypeAndKind(fir::FirOpBuilder &firOpBuilder,
mlir::Operation *op = mod.lookupSymbol(converter.mangleName(sym));
auto declareTargetOp =
llvm::dyn_cast_if_present<mlir::omp::DeclareTargetInterface>(op);
+
+ // Double check it's not part of a common block, and that the common block
+ // isn't marked declare target.
+ if (!declareTargetOp) {
+ if (const semantics::Symbol *common =
+ semantics::FindCommonBlockContaining(sym.GetUltimate())) {
+ mlir::Operation *commonOp =
+ mod.lookupSymbol(converter.mangleName(*common));
+ declareTargetOp =
+ llvm::dyn_cast_if_present<mlir::omp::DeclareTargetInterface>(
+ commonOp);
+ }
+ }
+
if (declareTargetOp && declareTargetOp.isDeclareTarget()) {
if (declareTargetOp.getDeclareTargetCaptureClause() ==
mlir::omp::DeclareTargetCaptureClause::link &&
@@ -1414,6 +1428,16 @@ static void genBodyOfTargetOp(
if (HostEvalInfo *hostEvalInfo = getHostEvalInfoStackTop(converter))
hostEvalInfo->bindOperands(argIface.getHostEvalBlockArgs());
+ // If we map a common block using it's symbol e.g. map(tofrom: /common_block/)
+ // and accessing its members within the target region, there is a large
+ // chance we will end up with uses external to the region accessing the common
+ // resolve these, we do so by generating new common block member accesses
+ // within the region, binding them to the member symbol for the scope of the
+ // region so that subsequent code generation within the region will utilise
+ // our new member accesses we have created.
+ genIntermediateCommonBlockAccessors(
+ converter, currentLocation, argIface.getMapBlockArgs(), args.map.syms);
+
// Check if cloning the bounds introduced any dependency on the outer region.
// If so, then either clone them as well if they are MemoryEffectFree, or else
// copy them to a new temporary and add them to the map and block_argument
@@ -1440,16 +1464,6 @@ static void genBodyOfTargetOp(
// Create the insertion point after the marker.
firOpBuilder.setInsertionPointAfter(undefMarker.getDefiningOp());
- // If we map a common block using it's symbol e.g. map(tofrom: /common_block/)
- // and accessing its members within the target region, there is a large
- // chance we will end up with uses external to the region accessing the common
- // resolve these, we do so by generating new common block member accesses
- // within the region, binding them to the member symbol for the scope of the
- // region so that subsequent code generation within the region will utilise
- // our new member accesses we have created.
- genIntermediateCommonBlockAccessors(
- converter, currentLocation, argIface.getMapBlockArgs(), args.map.syms);
-
if (ConstructQueue::const_iterator next = std::next(item);
next != queue.end()) {
genOMPDispatch(converter, symTable, semaCtx, eval, currentLocation, queue,
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 96f66246ac676..8e122a24cfbb9 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -942,6 +942,47 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
void Post(const parser::EndLabel &endLabel) { CheckSourceLabel(endLabel.v); }
void Post(const parser::EorLabel &eorLabel) { CheckSourceLabel(eorLabel.v); }
+ void ResolveOmpObjectsForMapClause(
+ Symbol::Flag mapFlag, const parser::OmpObjectList &objList) {
+ for (const auto &ompObj : objList.v) {
+ common::visit(
+ common::visitors{
+ [&](const parser::Designator &designator) {
+ if (const auto *name{
+ parser::GetDesignatorNameIfDataRef(designator)}) {
+ if (name->symbol) {
+ name->symbol->set(mapFlag);
+ }
+ }
+ },
+ [&](const auto &name) {},
+ },
+ ompObj.u);
+
+ ResolveOmpObject(ompObj, mapFlag);
+ }
+ }
+
+ void Post(const parser::OmpFromClause &x) {
+ const auto &ompObjList{*parser::omp::GetOmpObjectList(x)};
+ ResolveOmpObjectsForMapClause(Symbol::Flag::OmpMapFrom, ompObjList);
+ }
+
+ void Post(const parser::OmpToClause &x) {
+ // This is different from the parser::OmpFromClause case, as this to applies
+ // to both declare target to, and update to, so slightly different handling
+ // is required in that we must exit early to avoid applying extra symbol
+ // flags. This is reasonable for now, but if we wish to apply this
+ // resolution to declare target to (and likely enter in another function
+ // like this) we will have to extend the handling to act differently for
+ // declare target rather than simply return.
+ if (GetContext().directive == llvm::omp::Directive::OMPD_declare_target)
+ return;
+
+ const auto &ompObjList{*parser::omp::GetOmpObjectList(x)};
+ ResolveOmpObjectsForMapClause(Symbol::Flag::OmpMapTo, ompObjList);
+ }
+
void Post(const parser::OmpMapClause &x) {
unsigned version{context_.langOptions().OpenMPVersion};
std::optional<Symbol::Flag> ompFlag;
@@ -3219,13 +3260,13 @@ void OmpAttributeVisitor::ResolveOmpDesignator(
void OmpAttributeVisitor::ResolveOmpCommonBlock(
const parser::Name &name, Symbol::Flag ompFlag) {
if (auto *symbol{ResolveOmpCommonBlockName(&name)}) {
+ auto &details{symbol->get<CommonBlockDetails>()};
if (!dataCopyingAttributeFlags.test(ompFlag)) {
CheckMultipleAppearances(name, *symbol, Symbol::Flag::OmpCommonBlock);
}
// 2.15.3 When a named common block appears in a list, it has the
// same meaning as if every explicit member of the common block
// appeared in the list
- auto &details{symbol->get<CommonBlockDetails>()};
for (auto [index, object] : llvm::enumerate(details.objects())) {
if (auto *resolvedObject{ResolveOmp(*object, ompFlag, currScope())}) {
if (dataCopyingAttributeFlags.test(ompFlag)) {
diff --git a/flang/test/Lower/OpenMP/common-block-target-update.f90 b/flang/test/Lower/OpenMP/common-block-target-update.f90
new file mode 100644
index 0000000000000..ff328e3335633
--- /dev/null
+++ b/flang/test/Lower/OpenMP/common-block-target-update.f90
@@ -0,0 +1,30 @@
+!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+
+program main
+ implicit none
+ common/cb_array/array
+ real(kind=8), dimension(100) :: array
+ integer i
+
+ !$omp target update to(/cb_array/)
+
+ !$omp target teams distribute parallel do map(tofrom: /cb_array/)
+ do i = 1, 100
+ array(i) = i
+ enddo
+ !$omp end target teams distribute parallel do
+
+ !$omp target update from(/cb_array/)
+end program
+
+! CHECK-LABEL: func.func @_QQmain()
+
+! CHECK: %[[CB_ADDR:.*]] = fir.address_of(@cb_array_) : !fir.ref<!fir.array<800xi8>>
+! CHECK: %[[MAP_TO:.*]] = omp.map.info var_ptr(%[[CB_ADDR]] : !fir.ref<!fir.array<800xi8>>, !fir.array<800xi8>) map_clauses(to) capture(ByRef) -> !fir.ref<!fir.array<800xi8>> {name = "cb_array"}
+! CHECK: omp.target_update map_entries(%[[MAP_TO]] : !fir.ref<!fir.array<800xi8>>)
+
+! CHECK: %[[MAP_TOFROM:.*]] = omp.map.info var_ptr(%[[CB_ADDR]] : !fir.ref<!fir.array<800xi8>>, !fir.array<800xi8>) map_clauses(tofrom) capture(ByRef) -> !fir.ref<!fir.array<800xi8>> {name = "cb_array"}
+! CHECK: omp.target {{.*}} map_entries(%[[MAP_TOFROM]] -> %{{.*}}, %{{.*}} -> %{{.*}} : !fir.ref<!fir.array<800xi8>>, !fir.ref<i32>)
+
+! CHECK: %[[MAP_FROM:.*]] = omp.map.info var_ptr(%[[CB_ADDR]] : !fir.ref<!fir.array<800xi8>>, !fir.array<800xi8>) map_clauses(from) capture(ByRef) -> !fir.ref<!fir.array<800xi8>> {name = "cb_array"}
+! CHECK: omp.target_update map_entries(%[[MAP_FROM]] : !fir.ref<!fir.array<800xi8>>)
``````````
</details>
https://github.com/llvm/llvm-project/pull/187221
More information about the flang-commits
mailing list