[NFC][Flang] Move bounds helper functions to Util header. #154164
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
flang
There was a problem hiding this comment.
Pull Request Overview
This PR refactors OpenMP-related bounds helper functions by moving them from individual source files to a centralized utility header for better code reuse and maintainability.
- Moves
needsBoundsOpsandgenBoundsOpsfunctions from implementation files toflang/include/flang/Optimizer/OpenMP/Utils.h - Updates function calls across multiple files to use the new
flangomp::namespace - Enhances the
genBoundsOpsfunction signature with optional parameters for better flexibility
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp | Removes duplicate helper functions and updates calls to use centralized utilities |
| flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp | Removes duplicate helper functions and updates calls to use centralized utilities |
| flang/lib/Lower/OpenMP/OpenMP.cpp | Updates function calls to use new centralized utilities with enhanced parameters |
| flang/include/flang/Optimizer/OpenMP/Utils.h | Adds the moved helper functions with improved signatures and documentation |
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Akash Banerjee (TIFitis) ChangesThis PR moves the Full diff: https://github.com/llvm/llvm-project/pull/154164.diff 4 Files Affected:
diff --git a/flang/include/flang/Optimizer/OpenMP/Utils.h b/flang/include/flang/Optimizer/OpenMP/Utils.h
index 636c768b016b7..bbaad30a0ad20 100644
--- a/flang/include/flang/Optimizer/OpenMP/Utils.h
+++ b/flang/include/flang/Optimizer/OpenMP/Utils.h
@@ -13,6 +13,17 @@
#ifndef FORTRAN_OPTIMIZER_OPENMP_UTILS_H
#define FORTRAN_OPTIMIZER_OPENMP_UTILS_H
+#include "flang/Optimizer/Builder/BoxValue.h"
+#include "flang/Optimizer/Builder/DirectivesCommon.h"
+#include "flang/Optimizer/Builder/FIRBuilder.h"
+#include "flang/Optimizer/Builder/HLFIRTools.h"
+#include "flang/Optimizer/Dialect/FIRType.h"
+
+#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
+#include "mlir/IR/Value.h"
+
+#include "llvm/ADT/SmallVector.h"
+
namespace flangomp {
enum class DoConcurrentMappingKind {
@@ -21,6 +32,35 @@ enum class DoConcurrentMappingKind {
DCMK_Device ///< Lower to run in parallel on the GPU.
};
+/// Return true if the variable has a dynamic size and therefore requires
+/// bounds operations to describe its extents.
+inline bool needsBoundsOps(mlir::Value var) {
+ assert(mlir::isa<mlir::omp::PointerLikeType>(var.getType()) &&
+ "needsBoundsOps can deal only with pointer types");
+ mlir::Type t = fir::unwrapRefType(var.getType());
+ if (mlir::Type inner = fir::dyn_cast_ptrOrBoxEleTy(t))
+ return fir::hasDynamicSize(inner);
+ return fir::hasDynamicSize(t);
+}
+
+/// Generate MapBoundsOp operations for the variable if required and append them
+/// to `boundsOps`.
+inline llvm::SmallVector<mlir::Value> genBoundsOps(fir::FirOpBuilder &builder,
+ mlir::Value var,
+ bool isAssumedSize = false,
+ bool isOptional = false) {
+ mlir::Location loc = var.getLoc();
+ fir::factory::AddrAndBoundsInfo info =
+ fir::factory::getDataOperandBaseAddr(builder, var, isOptional, loc);
+ fir::ExtendedValue exv =
+ hlfir::translateToExtendedValue(loc, builder, hlfir::Entity{info.addr},
+ /*contiguousHint=*/true)
+ .first;
+ return fir::factory::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
+ mlir::omp::MapBoundsType>(
+ builder, info, exv, isAssumedSize, loc);
+}
+
} // namespace flangomp
#endif // FORTRAN_OPTIMIZER_OPENMP_UTILS_H
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index fef64ccc15015..ed9730d5d552c 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -30,6 +30,7 @@
#include "flang/Optimizer/Builder/Todo.h"
#include "flang/Optimizer/Dialect/FIRType.h"
#include "flang/Optimizer/HLFIR/HLFIROps.h"
+#include "flang/Optimizer/OpenMP/Utils.h"
#include "flang/Parser/characters.h"
#include "flang/Parser/openmp-utils.h"
#include "flang/Parser/parse-tree.h"
@@ -1457,17 +1458,8 @@ static void genBodyOfTargetOp(
firOpBuilder.createTemporary(val.getLoc(), val.getType());
firOpBuilder.createStoreWithConvert(copyVal.getLoc(), val, copyVal);
- fir::factory::AddrAndBoundsInfo info =
- fir::factory::getDataOperandBaseAddr(
- firOpBuilder, val, /*isOptional=*/false, val.getLoc());
llvm::SmallVector<mlir::Value> bounds =
- fir::factory::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
- mlir::omp::MapBoundsType>(
- firOpBuilder, info,
- hlfir::translateToExtendedValue(val.getLoc(), firOpBuilder,
- hlfir::Entity{val})
- .first,
- /*dataExvIsAssumedSize=*/false, val.getLoc());
+ flangomp::genBoundsOps(firOpBuilder, val);
std::stringstream name;
firOpBuilder.setInsertionPoint(targetOp);
@@ -2583,12 +2575,10 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
Fortran::lower::getDataOperandBaseAddr(
converter, firOpBuilder, sym.GetUltimate(),
converter.getCurrentLocation());
- llvm::SmallVector<mlir::Value> bounds =
- fir::factory::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
- mlir::omp::MapBoundsType>(
- firOpBuilder, info, dataExv,
- semantics::IsAssumedSizeArray(sym.GetUltimate()),
- converter.getCurrentLocation());
+ llvm::SmallVector<mlir::Value> bounds = flangomp::genBoundsOps(
+ firOpBuilder, info.rawInput,
+ semantics::IsAssumedSizeArray(sym.GetUltimate()),
+ semantics::IsOptional(sym.GetUltimate()));
mlir::Value baseOp = info.rawInput;
mlir::Type eleType = baseOp.getType();
if (auto refType = mlir::dyn_cast<fir::ReferenceType>(baseOp.getType()))
diff --git a/flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp b/flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp
index 8b9991301aae8..abc0626affc6d 100644
--- a/flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp
+++ b/flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp
@@ -6,16 +6,14 @@
//
//===----------------------------------------------------------------------===//
-#include "flang/Optimizer/Builder/DirectivesCommon.h"
#include "flang/Optimizer/Builder/FIRBuilder.h"
#include "flang/Optimizer/Builder/HLFIRTools.h"
#include "flang/Optimizer/Dialect/FIROps.h"
#include "flang/Optimizer/Dialect/FIRType.h"
#include "flang/Optimizer/Dialect/Support/KindMapping.h"
#include "flang/Optimizer/HLFIR/HLFIROps.h"
+#include "flang/Optimizer/OpenMP/Utils.h"
-#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
-#include "mlir/Dialect/OpenMP/OpenMPInterfaces.h"
#include "mlir/IR/BuiltinAttributes.h"
#include "mlir/IR/Operation.h"
#include "mlir/Pass/Pass.h"
@@ -33,36 +31,6 @@ namespace {
class AutomapToTargetDataPass
: public flangomp::impl::AutomapToTargetDataPassBase<
AutomapToTargetDataPass> {
-
- // Returns true if the variable has a dynamic size and therefore requires
- // bounds operations to describe its extents.
- inline bool needsBoundsOps(mlir::Value var) {
- assert(mlir::isa<mlir::omp::PointerLikeType>(var.getType()) &&
- "only pointer like types expected");
- mlir::Type t = fir::unwrapRefType(var.getType());
- if (mlir::Type inner = fir::dyn_cast_ptrOrBoxEleTy(t))
- return fir::hasDynamicSize(inner);
- return fir::hasDynamicSize(t);
- }
-
- // Generate MapBoundsOp operations for the variable if required.
- inline void genBoundsOps(fir::FirOpBuilder &builder, mlir::Value var,
- llvm::SmallVectorImpl<mlir::Value> &boundsOps) {
- mlir::Location loc = var.getLoc();
- fir::factory::AddrAndBoundsInfo info =
- fir::factory::getDataOperandBaseAddr(builder, var,
- /*isOptional=*/false, loc);
- fir::ExtendedValue exv =
- hlfir::translateToExtendedValue(loc, builder, hlfir::Entity{info.addr},
- /*contiguousHint=*/true)
- .first;
- llvm::SmallVector<mlir::Value> tmp =
- fir::factory::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
- mlir::omp::MapBoundsType>(
- builder, info, exv, /*dataExvIsAssumedSize=*/false, loc);
- llvm::append_range(boundsOps, tmp);
- }
-
void findRelatedAllocmemFreemem(fir::AddrOfOp addressOfOp,
llvm::DenseSet<fir::StoreOp> &allocmems,
llvm::DenseSet<fir::LoadOp> &freemems) {
@@ -112,8 +80,8 @@ class AutomapToTargetDataPass
auto addMapInfo = [&](auto globalOp, auto memOp) {
builder.setInsertionPointAfter(memOp);
SmallVector<Value> bounds;
- if (needsBoundsOps(memOp.getMemref()))
- genBoundsOps(builder, memOp.getMemref(), bounds);
+ if (flangomp::needsBoundsOps(memOp.getMemref()))
+ bounds = flangomp::genBoundsOps(builder, memOp.getMemref());
omp::TargetEnterExitUpdateDataOperands clauses;
mlir::omp::MapInfoOp mapInfo = mlir::omp::MapInfoOp::create(
diff --git a/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp b/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp
index 970f7d7ab063f..784967609c4f7 100644
--- a/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp
+++ b/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp
@@ -22,13 +22,13 @@
// 2. Generalize this for more than just omp.target ops.
//===----------------------------------------------------------------------===//
-#include "flang/Optimizer/Builder/DirectivesCommon.h"
#include "flang/Optimizer/Builder/FIRBuilder.h"
#include "flang/Optimizer/Builder/HLFIRTools.h"
#include "flang/Optimizer/Dialect/FIRType.h"
#include "flang/Optimizer/Dialect/Support/KindMapping.h"
#include "flang/Optimizer/HLFIR/HLFIROps.h"
#include "flang/Optimizer/OpenMP/Passes.h"
+#include "flang/Optimizer/OpenMP/Utils.h"
#include "mlir/Dialect/Func/IR/FuncOps.h"
#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
@@ -105,8 +105,8 @@ class MapsForPrivatizedSymbolsPass
// Figure out the bounds because knowing the bounds will help the subsequent
// MapInfoFinalizationPass map the underlying data of the descriptor.
llvm::SmallVector<mlir::Value> boundsOps;
- if (needsBoundsOps(varPtr))
- genBoundsOps(builder, varPtr, boundsOps);
+ if (flangomp::needsBoundsOps(varPtr))
+ boundsOps = flangomp::genBoundsOps(builder, varPtr);
mlir::omp::VariableCaptureKind captureKind =
mlir::omp::VariableCaptureKind::ByRef;
@@ -193,38 +193,5 @@ class MapsForPrivatizedSymbolsPass
}
}
}
- // As the name suggests, this function examines var to determine if
- // it has dynamic size. If true, this pass'll have to extract these
- // bounds from descriptor of var and add the bounds to the resultant
- // MapInfoOp.
- bool needsBoundsOps(mlir::Value var) {
- assert(mlir::isa<omp::PointerLikeType>(var.getType()) &&
- "needsBoundsOps can deal only with pointer types");
- mlir::Type t = fir::unwrapRefType(var.getType());
- // t could be a box, so look inside the box
- auto innerType = fir::dyn_cast_ptrOrBoxEleTy(t);
- if (innerType)
- return fir::hasDynamicSize(innerType);
- return fir::hasDynamicSize(t);
- }
-
- void genBoundsOps(fir::FirOpBuilder &builder, mlir::Value var,
- llvm::SmallVector<mlir::Value> &boundsOps) {
- mlir::Location loc = var.getLoc();
- fir::factory::AddrAndBoundsInfo info =
- fir::factory::getDataOperandBaseAddr(builder, var,
- /*isOptional=*/false, loc);
- fir::ExtendedValue extendedValue =
- hlfir::translateToExtendedValue(loc, builder, hlfir::Entity{info.addr},
- /*continguousHint=*/true)
- .first;
- llvm::SmallVector<mlir::Value> boundsOpsVec =
- fir::factory::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
- mlir::omp::MapBoundsType>(
- builder, info, extendedValue,
- /*dataExvIsAssumedSize=*/false, loc);
- for (auto bounds : boundsOpsVec)
- boundsOps.push_back(bounds);
- }
};
} // namespace
|
bhandarkar-pranav
approved these changes
Aug 21, 2025
This PR moves the needsBoundsOps and genBoundsOps helper functions to flang/include/flang/Optimizer/OpenMP/Utils.h.
TIFitis
force-pushed
the
bounds_util
branch
from
e739ac5 to
eed67c4
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/204/builds/22049 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/203/builds/23237 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/205/builds/22026 Here is the relevant piece of the build log for the reference |
TIFitis
added a commit
that referenced
this pull request
Sep 15, 2025
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Sep 16, 2025
…header." (#158654) Reverts llvm/llvm-project#154164
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves the
needsBoundsOpsandgenBoundsOpshelper functions toflang/include/flang/Optimizer/OpenMP/Utils.h.