[flang][OpenMP] Implicitly map allocatable record fields #117867
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
ergawy
changed the title
llvmbot
added
flang
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-flang-fir-hlfir Author: Kareem Ergawy (ergawy) ChangesThis is a proof-of-concept to implicitely map allocatable record fields. This PR contains the following changes:
If the taken approach by this PR is acceptable, I will convert it to a proper PR and add more extensive testing. Full diff: https://github.com/llvm/llvm-project/pull/117867.diff 5 Files Affected:
diff --git a/flang/lib/Lower/DirectivesCommon.h b/flang/lib/Lower/DirectivesCommon.h
index 88514b16743278..6e2c6ee4b1bcdb 100644
--- a/flang/lib/Lower/DirectivesCommon.h
+++ b/flang/lib/Lower/DirectivesCommon.h
@@ -609,11 +609,10 @@ void createEmptyRegionBlocks(
}
}
-inline AddrAndBoundsInfo
-getDataOperandBaseAddr(Fortran::lower::AbstractConverter &converter,
- fir::FirOpBuilder &builder,
- Fortran::lower::SymbolRef sym, mlir::Location loc) {
- mlir::Value symAddr = converter.getSymbolAddress(sym);
+inline AddrAndBoundsInfo getDataOperandBaseAddr(fir::FirOpBuilder &builder,
+ mlir::Value symAddr,
+ bool isOptional,
+ mlir::Location loc) {
mlir::Value rawInput = symAddr;
if (auto declareOp =
mlir::dyn_cast_or_null<hlfir::DeclareOp>(symAddr.getDefiningOp())) {
@@ -621,20 +620,11 @@ getDataOperandBaseAddr(Fortran::lower::AbstractConverter &converter,
rawInput = declareOp.getResults()[1];
}
- // TODO: Might need revisiting to handle for non-shared clauses
- if (!symAddr) {
- if (const auto *details =
- sym->detailsIf<Fortran::semantics::HostAssocDetails>()) {
- symAddr = converter.getSymbolAddress(details->symbol());
- rawInput = symAddr;
- }
- }
-
if (!symAddr)
llvm::report_fatal_error("could not retrieve symbol address");
mlir::Value isPresent;
- if (Fortran::semantics::IsOptional(sym))
+ if (isOptional)
isPresent =
builder.create<fir::IsPresentOp>(loc, builder.getI1Type(), rawInput);
@@ -648,8 +638,7 @@ getDataOperandBaseAddr(Fortran::lower::AbstractConverter &converter,
// all address/dimension retrievals. For Fortran optional though, leave
// the load generation for later so it can be done in the appropriate
// if branches.
- if (mlir::isa<fir::ReferenceType>(symAddr.getType()) &&
- !Fortran::semantics::IsOptional(sym)) {
+ if (mlir::isa<fir::ReferenceType>(symAddr.getType()) && !isOptional) {
mlir::Value addr = builder.create<fir::LoadOp>(loc, symAddr);
return AddrAndBoundsInfo(addr, rawInput, isPresent, boxTy);
}
@@ -659,6 +648,14 @@ getDataOperandBaseAddr(Fortran::lower::AbstractConverter &converter,
return AddrAndBoundsInfo(symAddr, rawInput, isPresent);
}
+inline AddrAndBoundsInfo
+getDataOperandBaseAddr(Fortran::lower::AbstractConverter &converter,
+ fir::FirOpBuilder &builder,
+ Fortran::lower::SymbolRef sym, mlir::Location loc) {
+ return getDataOperandBaseAddr(builder, converter.getSymbolAddress(sym),
+ Fortran::semantics::IsOptional(sym), loc);
+}
+
template <typename BoundsOp, typename BoundsType>
llvm::SmallVector<mlir::Value>
gatherBoundsOrBoundValues(fir::FirOpBuilder &builder, mlir::Location loc,
@@ -1224,6 +1221,25 @@ AddrAndBoundsInfo gatherDataOperandAddrAndBounds(
return info;
}
+
+template <typename BoundsOp, typename BoundsType>
+llvm::SmallVector<mlir::Value>
+genImplicitBoundsOps(fir::FirOpBuilder &builder, lower::AddrAndBoundsInfo &info,
+ fir::ExtendedValue dataExv, bool dataExvIsAssumedSize,
+ mlir::Location loc) {
+ llvm::SmallVector<mlir::Value> bounds;
+
+ mlir::Value baseOp = info.rawInput;
+ if (mlir::isa<fir::BaseBoxType>(fir::unwrapRefType(baseOp.getType())))
+ bounds = lower::genBoundsOpsFromBox<BoundsOp, BoundsType>(builder, loc,
+ dataExv, info);
+ if (mlir::isa<fir::SequenceType>(fir::unwrapRefType(baseOp.getType()))) {
+ bounds = lower::genBaseBoundsOps<BoundsOp, BoundsType>(
+ builder, loc, dataExv, dataExvIsAssumedSize);
+ }
+
+ return bounds;
+}
} // namespace lower
} // namespace Fortran
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index ea2d1eb66bfea5..ddf15ce7f3c3dd 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -1817,32 +1817,25 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
if (const auto *details =
sym.template detailsIf<semantics::HostAssocDetails>())
converter.copySymbolBinding(details->symbol(), sym);
- llvm::SmallVector<mlir::Value> bounds;
std::stringstream name;
fir::ExtendedValue dataExv = converter.getSymbolExtendedValue(sym);
name << sym.name().ToString();
lower::AddrAndBoundsInfo info = getDataOperandBaseAddr(
converter, firOpBuilder, sym, converter.getCurrentLocation());
- mlir::Value baseOp = info.rawInput;
- if (mlir::isa<fir::BaseBoxType>(fir::unwrapRefType(baseOp.getType())))
- bounds = lower::genBoundsOpsFromBox<mlir::omp::MapBoundsOp,
- mlir::omp::MapBoundsType>(
- firOpBuilder, converter.getCurrentLocation(), dataExv, info);
- if (mlir::isa<fir::SequenceType>(fir::unwrapRefType(baseOp.getType()))) {
- bool dataExvIsAssumedSize =
- semantics::IsAssumedSizeArray(sym.GetUltimate());
- bounds = lower::genBaseBoundsOps<mlir::omp::MapBoundsOp,
- mlir::omp::MapBoundsType>(
- firOpBuilder, converter.getCurrentLocation(), dataExv,
- dataExvIsAssumedSize);
- }
+ llvm::SmallVector<mlir::Value> bounds =
+ lower::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
+ mlir::omp::MapBoundsType>(
+ firOpBuilder, info, dataExv,
+ semantics::IsAssumedSizeArray(sym.GetUltimate()),
+ converter.getCurrentLocation());
llvm::omp::OpenMPOffloadMappingFlags mapFlag =
llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_IMPLICIT;
mlir::omp::VariableCaptureKind captureKind =
mlir::omp::VariableCaptureKind::ByRef;
+ mlir::Value baseOp = info.rawInput;
mlir::Type eleType = baseOp.getType();
if (auto refType = mlir::dyn_cast<fir::ReferenceType>(baseOp.getType()))
eleType = refType.getElementType();
diff --git a/flang/lib/Optimizer/OpenMP/CMakeLists.txt b/flang/lib/Optimizer/OpenMP/CMakeLists.txt
index b1e0dbf6e707e5..0b78773a252cd2 100644
--- a/flang/lib/Optimizer/OpenMP/CMakeLists.txt
+++ b/flang/lib/Optimizer/OpenMP/CMakeLists.txt
@@ -11,6 +11,7 @@ add_flang_library(FlangOpenMPTransforms
FIRDialect
HLFIROpsIncGen
FlangOpenMPPassesIncGen
+ ${dialect_libs}
LINK_LIBS
FIRAnalysis
@@ -25,4 +26,5 @@ add_flang_library(FlangOpenMPTransforms
HLFIRDialect
MLIRIR
MLIRPass
+ ${dialect_libs}
)
diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
index 4575c90e34acdd..717d0dcd3fdfba 100644
--- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
+++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
@@ -25,9 +25,12 @@
//===----------------------------------------------------------------------===//
#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 "mlir/Analysis/SliceAnalysis.h"
#include "mlir/Dialect/Func/IR/FuncOps.h"
#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
#include "mlir/IR/BuiltinDialect.h"
@@ -43,6 +46,10 @@
#include <iterator>
#include <numeric>
+// Move to `flang/include/flang/Common/OpenMP-utils.h` once
+// https://github.com/llvm/llvm-project/pull/115443 is merge.
+#include "../../Lower/DirectivesCommon.h"
+
namespace flangomp {
#define GEN_PASS_DEF_MAPINFOFINALIZATIONPASS
#include "flang/Optimizer/OpenMP/Passes.h.inc"
@@ -485,6 +492,153 @@ class MapInfoFinalizationPass
// clear all local allocations we made for any boxes in any prior
// iterations from previous function scopes.
localBoxAllocas.clear();
+ func->walk([&](mlir::omp::MapInfoOp op) {
+ mlir::Type underlyingType =
+ fir::unwrapRefType(op.getVarPtr().getType());
+
+ if (!fir::isRecordWithAllocatableMember(underlyingType))
+ return mlir::WalkResult::advance();
+
+ mlir::omp::TargetOp target =
+ mlir::dyn_cast_if_present<mlir::omp::TargetOp>(
+ getFirstTargetUser(op));
+
+ if (!target)
+ return mlir::WalkResult::advance();
+
+ auto mapClauseOwner =
+ llvm::dyn_cast<mlir::omp::MapClauseOwningOpInterface>(*target);
+
+ // TODO Add as a method to MapClauseOwningOpInterface.
+ unsigned mapVarIdx = 0;
+ for (auto [idx, mapOp] : llvm::enumerate(mapClauseOwner.getMapVars())) {
+ if (mapOp == op) {
+ mapVarIdx = idx;
+ break;
+ }
+ }
+
+ auto argIface =
+ llvm::dyn_cast<mlir::omp::BlockArgOpenMPOpInterface>(*target);
+ mlir::BlockArgument opBlockArg = argIface.getMapBlockArgs()[mapVarIdx];
+ llvm::SetVector<mlir::Operation *> mapVarForwardSlice;
+ mlir::getForwardSlice(opBlockArg, &mapVarForwardSlice);
+
+ mapVarForwardSlice.remove_if([&](mlir::Operation *sliceOp) {
+ // TODO Support coordinate_of ops.
+ //
+ // TODO Support call ops by recursively examining the forward slice of
+ // the corresponding paramemter to the field.
+ return !mlir::isa<hlfir::DesignateOp>(sliceOp);
+ });
+
+ auto recordType = mlir::cast<fir::RecordType>(underlyingType);
+ llvm::SmallVector<mlir::Value> newMapOpsForFields;
+ llvm::SmallVector<int64_t> fieldIdices;
+
+ for (auto fieldMemTyPair : recordType.getTypeList()) {
+ auto &field = fieldMemTyPair.first;
+ auto memTy = fieldMemTyPair.second;
+
+ bool shouldMapField =
+ llvm::find_if(mapVarForwardSlice, [&](mlir::Operation *sliceOp) {
+ if (!fir::isAllocatableType(memTy))
+ return false;
+
+ auto designateOp = mlir::dyn_cast<hlfir::DesignateOp>(sliceOp);
+ if (!designateOp)
+ return false;
+
+ return designateOp.getComponent() &&
+ designateOp.getComponent()->strref() == field;
+ }) != mapVarForwardSlice.end();
+
+ // TODO Handle recursive record types.
+
+ if (!shouldMapField)
+ continue;
+
+ int64_t fieldIdx = recordType.getFieldIndex(field);
+ bool alreadyMapped = false;
+
+ if (op.getMembersIndexAttr())
+ for (auto indexList : op.getMembersIndexAttr()) {
+ auto indexListAttr = mlir::cast<mlir::ArrayAttr>(indexList);
+ if (indexListAttr.size() == 1 &&
+ mlir::cast<mlir::IntegerAttr>(indexListAttr[0]).getInt() ==
+ fieldIdx)
+ alreadyMapped = true;
+ }
+
+ if (alreadyMapped)
+ continue;
+
+ builder.setInsertionPoint(op);
+ mlir::Value fieldIdxVal = builder.createIntegerConstant(
+ op.getLoc(), mlir::IndexType::get(builder.getContext()),
+ fieldIdx);
+ auto fieldCoord = builder.create<fir::CoordinateOp>(
+ op.getLoc(), builder.getRefType(memTy), op.getVarPtr(),
+ fieldIdxVal);
+ Fortran::lower::AddrAndBoundsInfo info =
+ Fortran::lower::getDataOperandBaseAddr(
+ builder, fieldCoord, /*isOptional=*/false, op.getLoc());
+ llvm::SmallVector<mlir::Value> bounds =
+ Fortran::lower::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
+ mlir::omp::MapBoundsType>(
+ builder, info,
+ hlfir::translateToExtendedValue(op.getLoc(), builder,
+ hlfir::Entity{fieldCoord})
+ .first,
+ /*dataExvIsAssumedSize=*/false, op.getLoc());
+
+ mlir::omp::MapInfoOp fieldMapOp =
+ builder.create<mlir::omp::MapInfoOp>(
+ op.getLoc(), fieldCoord.getResult().getType(),
+ fieldCoord.getResult(),
+ mlir::TypeAttr::get(
+ fir::unwrapRefType(fieldCoord.getResult().getType())),
+ /*varPtrPtr=*/mlir::Value{},
+ /*members=*/mlir::ValueRange{},
+ /*members_index=*/mlir::ArrayAttr{},
+ /*bounds=*/bounds, op.getMapTypeAttr(),
+ builder.getAttr<mlir::omp::VariableCaptureKindAttr>(
+ mlir::omp::VariableCaptureKind::ByRef),
+ builder.getStringAttr(op.getNameAttr().strref() + "." +
+ field + ".implicit_map"),
+ /*partial_map=*/builder.getBoolAttr(false));
+ newMapOpsForFields.emplace_back(fieldMapOp);
+ fieldIdices.emplace_back(fieldIdx);
+ }
+
+ if (!newMapOpsForFields.empty()) {
+ op.getMembersMutable().append(newMapOpsForFields);
+ llvm::SmallVector<llvm::SmallVector<int64_t>> newMemberIndices;
+ mlir::ArrayAttr oldMembersIdxAttr = op.getMembersIndexAttr();
+
+ if (oldMembersIdxAttr)
+ for (mlir::Attribute indexList : oldMembersIdxAttr) {
+ llvm::SmallVector<int64_t> listVec;
+
+ for (mlir::Attribute index :
+ mlir::cast<mlir::ArrayAttr>(indexList))
+ listVec.push_back(
+ mlir::cast<mlir::IntegerAttr>(index).getInt());
+
+ newMemberIndices.emplace_back(std::move(listVec));
+ }
+
+ for (int64_t newFieldIdx : fieldIdices) {
+ newMemberIndices.emplace_back(
+ llvm::SmallVector<int64_t>(1, newFieldIdx));
+ }
+
+ op.setMembersIndexAttr(
+ builder.create2DI64ArrayAttr(newMemberIndices));
+ }
+
+ return mlir::WalkResult::advance();
+ });
func->walk([&](mlir::omp::MapInfoOp op) {
// TODO: Currently only supports a single user for the MapInfoOp. This
diff --git a/offload/test/offloading/fortran/implicit-record-field-mapping.f90 b/offload/test/offloading/fortran/implicit-record-field-mapping.f90
new file mode 100644
index 00000000000000..8bde033d4ef4c3
--- /dev/null
+++ b/offload/test/offloading/fortran/implicit-record-field-mapping.f90
@@ -0,0 +1,63 @@
+! Test implicit mapping of alloctable record fields.
+
+! REQUIRES: flang, amdgpu
+
+! This fails only because it needs the Fortran runtime built for device. If this
+! is avaialbe, this test succeedds when run.
+! XFAIL: *
+
+! RUN: %libomptarget-compile-fortran-generic
+! RUN: env LIBOMPTARGET_INFO=16 %libomptarget-run-generic 2>&1 | %fcheck-generic
+module record_m
+ implicit none
+
+ private
+ public :: recored_t
+
+ type recored_t
+ real, allocatable :: not_to_implicitly_map(:)
+ real, allocatable :: to_implicitly_map(:)
+ end type
+
+ interface recored_t
+ end interface
+end module record_m
+
+program concurrent_inferences
+ use record_m, only : recored_t
+ implicit none
+
+ type(recored_t) :: dst_record
+ real :: src_array(10)
+ real :: dst_sum, src_sum
+ integer :: i
+
+ call random_number(src_array)
+ dst_sum = 0
+ src_sum = 0
+
+ do i=1,10
+ src_sum = src_sum + src_array(i)
+ end do
+ print *, "src_sum=", src_sum
+
+ !$omp target map(from: dst_sum)
+ dst_record%to_implicitly_map = src_array
+ dst_sum = 0
+
+ do i=1,10
+ dst_sum = dst_sum + dst_record%to_implicitly_map(i)
+ end do
+ !$omp end target
+
+ print *, "dst_sum=", dst_sum
+
+ if (src_sum == dst_sum) then
+ print *, "Test succeeded!"
+ else
+ print *, "Test failed!", " dst_sum=", dst_sum, "vs. src_sum=", src_sum
+ endif
+end program
+
+! CHECK: "PluginInterface" device {{[0-9]+}} info: Launching kernel {{.*}}
+! CHECK: Test succeeded!
|
|
@llvm/pr-subscribers-offload Author: Kareem Ergawy (ergawy) ChangesThis is a proof-of-concept to implicitely map allocatable record fields. This PR contains the following changes:
If the taken approach by this PR is acceptable, I will convert it to a proper PR and add more extensive testing. Full diff: https://github.com/llvm/llvm-project/pull/117867.diff 5 Files Affected:
diff --git a/flang/lib/Lower/DirectivesCommon.h b/flang/lib/Lower/DirectivesCommon.h
index 88514b16743278..6e2c6ee4b1bcdb 100644
--- a/flang/lib/Lower/DirectivesCommon.h
+++ b/flang/lib/Lower/DirectivesCommon.h
@@ -609,11 +609,10 @@ void createEmptyRegionBlocks(
}
}
-inline AddrAndBoundsInfo
-getDataOperandBaseAddr(Fortran::lower::AbstractConverter &converter,
- fir::FirOpBuilder &builder,
- Fortran::lower::SymbolRef sym, mlir::Location loc) {
- mlir::Value symAddr = converter.getSymbolAddress(sym);
+inline AddrAndBoundsInfo getDataOperandBaseAddr(fir::FirOpBuilder &builder,
+ mlir::Value symAddr,
+ bool isOptional,
+ mlir::Location loc) {
mlir::Value rawInput = symAddr;
if (auto declareOp =
mlir::dyn_cast_or_null<hlfir::DeclareOp>(symAddr.getDefiningOp())) {
@@ -621,20 +620,11 @@ getDataOperandBaseAddr(Fortran::lower::AbstractConverter &converter,
rawInput = declareOp.getResults()[1];
}
- // TODO: Might need revisiting to handle for non-shared clauses
- if (!symAddr) {
- if (const auto *details =
- sym->detailsIf<Fortran::semantics::HostAssocDetails>()) {
- symAddr = converter.getSymbolAddress(details->symbol());
- rawInput = symAddr;
- }
- }
-
if (!symAddr)
llvm::report_fatal_error("could not retrieve symbol address");
mlir::Value isPresent;
- if (Fortran::semantics::IsOptional(sym))
+ if (isOptional)
isPresent =
builder.create<fir::IsPresentOp>(loc, builder.getI1Type(), rawInput);
@@ -648,8 +638,7 @@ getDataOperandBaseAddr(Fortran::lower::AbstractConverter &converter,
// all address/dimension retrievals. For Fortran optional though, leave
// the load generation for later so it can be done in the appropriate
// if branches.
- if (mlir::isa<fir::ReferenceType>(symAddr.getType()) &&
- !Fortran::semantics::IsOptional(sym)) {
+ if (mlir::isa<fir::ReferenceType>(symAddr.getType()) && !isOptional) {
mlir::Value addr = builder.create<fir::LoadOp>(loc, symAddr);
return AddrAndBoundsInfo(addr, rawInput, isPresent, boxTy);
}
@@ -659,6 +648,14 @@ getDataOperandBaseAddr(Fortran::lower::AbstractConverter &converter,
return AddrAndBoundsInfo(symAddr, rawInput, isPresent);
}
+inline AddrAndBoundsInfo
+getDataOperandBaseAddr(Fortran::lower::AbstractConverter &converter,
+ fir::FirOpBuilder &builder,
+ Fortran::lower::SymbolRef sym, mlir::Location loc) {
+ return getDataOperandBaseAddr(builder, converter.getSymbolAddress(sym),
+ Fortran::semantics::IsOptional(sym), loc);
+}
+
template <typename BoundsOp, typename BoundsType>
llvm::SmallVector<mlir::Value>
gatherBoundsOrBoundValues(fir::FirOpBuilder &builder, mlir::Location loc,
@@ -1224,6 +1221,25 @@ AddrAndBoundsInfo gatherDataOperandAddrAndBounds(
return info;
}
+
+template <typename BoundsOp, typename BoundsType>
+llvm::SmallVector<mlir::Value>
+genImplicitBoundsOps(fir::FirOpBuilder &builder, lower::AddrAndBoundsInfo &info,
+ fir::ExtendedValue dataExv, bool dataExvIsAssumedSize,
+ mlir::Location loc) {
+ llvm::SmallVector<mlir::Value> bounds;
+
+ mlir::Value baseOp = info.rawInput;
+ if (mlir::isa<fir::BaseBoxType>(fir::unwrapRefType(baseOp.getType())))
+ bounds = lower::genBoundsOpsFromBox<BoundsOp, BoundsType>(builder, loc,
+ dataExv, info);
+ if (mlir::isa<fir::SequenceType>(fir::unwrapRefType(baseOp.getType()))) {
+ bounds = lower::genBaseBoundsOps<BoundsOp, BoundsType>(
+ builder, loc, dataExv, dataExvIsAssumedSize);
+ }
+
+ return bounds;
+}
} // namespace lower
} // namespace Fortran
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index ea2d1eb66bfea5..ddf15ce7f3c3dd 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -1817,32 +1817,25 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
if (const auto *details =
sym.template detailsIf<semantics::HostAssocDetails>())
converter.copySymbolBinding(details->symbol(), sym);
- llvm::SmallVector<mlir::Value> bounds;
std::stringstream name;
fir::ExtendedValue dataExv = converter.getSymbolExtendedValue(sym);
name << sym.name().ToString();
lower::AddrAndBoundsInfo info = getDataOperandBaseAddr(
converter, firOpBuilder, sym, converter.getCurrentLocation());
- mlir::Value baseOp = info.rawInput;
- if (mlir::isa<fir::BaseBoxType>(fir::unwrapRefType(baseOp.getType())))
- bounds = lower::genBoundsOpsFromBox<mlir::omp::MapBoundsOp,
- mlir::omp::MapBoundsType>(
- firOpBuilder, converter.getCurrentLocation(), dataExv, info);
- if (mlir::isa<fir::SequenceType>(fir::unwrapRefType(baseOp.getType()))) {
- bool dataExvIsAssumedSize =
- semantics::IsAssumedSizeArray(sym.GetUltimate());
- bounds = lower::genBaseBoundsOps<mlir::omp::MapBoundsOp,
- mlir::omp::MapBoundsType>(
- firOpBuilder, converter.getCurrentLocation(), dataExv,
- dataExvIsAssumedSize);
- }
+ llvm::SmallVector<mlir::Value> bounds =
+ lower::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
+ mlir::omp::MapBoundsType>(
+ firOpBuilder, info, dataExv,
+ semantics::IsAssumedSizeArray(sym.GetUltimate()),
+ converter.getCurrentLocation());
llvm::omp::OpenMPOffloadMappingFlags mapFlag =
llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_IMPLICIT;
mlir::omp::VariableCaptureKind captureKind =
mlir::omp::VariableCaptureKind::ByRef;
+ mlir::Value baseOp = info.rawInput;
mlir::Type eleType = baseOp.getType();
if (auto refType = mlir::dyn_cast<fir::ReferenceType>(baseOp.getType()))
eleType = refType.getElementType();
diff --git a/flang/lib/Optimizer/OpenMP/CMakeLists.txt b/flang/lib/Optimizer/OpenMP/CMakeLists.txt
index b1e0dbf6e707e5..0b78773a252cd2 100644
--- a/flang/lib/Optimizer/OpenMP/CMakeLists.txt
+++ b/flang/lib/Optimizer/OpenMP/CMakeLists.txt
@@ -11,6 +11,7 @@ add_flang_library(FlangOpenMPTransforms
FIRDialect
HLFIROpsIncGen
FlangOpenMPPassesIncGen
+ ${dialect_libs}
LINK_LIBS
FIRAnalysis
@@ -25,4 +26,5 @@ add_flang_library(FlangOpenMPTransforms
HLFIRDialect
MLIRIR
MLIRPass
+ ${dialect_libs}
)
diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
index 4575c90e34acdd..717d0dcd3fdfba 100644
--- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
+++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
@@ -25,9 +25,12 @@
//===----------------------------------------------------------------------===//
#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 "mlir/Analysis/SliceAnalysis.h"
#include "mlir/Dialect/Func/IR/FuncOps.h"
#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
#include "mlir/IR/BuiltinDialect.h"
@@ -43,6 +46,10 @@
#include <iterator>
#include <numeric>
+// Move to `flang/include/flang/Common/OpenMP-utils.h` once
+// https://github.com/llvm/llvm-project/pull/115443 is merge.
+#include "../../Lower/DirectivesCommon.h"
+
namespace flangomp {
#define GEN_PASS_DEF_MAPINFOFINALIZATIONPASS
#include "flang/Optimizer/OpenMP/Passes.h.inc"
@@ -485,6 +492,153 @@ class MapInfoFinalizationPass
// clear all local allocations we made for any boxes in any prior
// iterations from previous function scopes.
localBoxAllocas.clear();
+ func->walk([&](mlir::omp::MapInfoOp op) {
+ mlir::Type underlyingType =
+ fir::unwrapRefType(op.getVarPtr().getType());
+
+ if (!fir::isRecordWithAllocatableMember(underlyingType))
+ return mlir::WalkResult::advance();
+
+ mlir::omp::TargetOp target =
+ mlir::dyn_cast_if_present<mlir::omp::TargetOp>(
+ getFirstTargetUser(op));
+
+ if (!target)
+ return mlir::WalkResult::advance();
+
+ auto mapClauseOwner =
+ llvm::dyn_cast<mlir::omp::MapClauseOwningOpInterface>(*target);
+
+ // TODO Add as a method to MapClauseOwningOpInterface.
+ unsigned mapVarIdx = 0;
+ for (auto [idx, mapOp] : llvm::enumerate(mapClauseOwner.getMapVars())) {
+ if (mapOp == op) {
+ mapVarIdx = idx;
+ break;
+ }
+ }
+
+ auto argIface =
+ llvm::dyn_cast<mlir::omp::BlockArgOpenMPOpInterface>(*target);
+ mlir::BlockArgument opBlockArg = argIface.getMapBlockArgs()[mapVarIdx];
+ llvm::SetVector<mlir::Operation *> mapVarForwardSlice;
+ mlir::getForwardSlice(opBlockArg, &mapVarForwardSlice);
+
+ mapVarForwardSlice.remove_if([&](mlir::Operation *sliceOp) {
+ // TODO Support coordinate_of ops.
+ //
+ // TODO Support call ops by recursively examining the forward slice of
+ // the corresponding paramemter to the field.
+ return !mlir::isa<hlfir::DesignateOp>(sliceOp);
+ });
+
+ auto recordType = mlir::cast<fir::RecordType>(underlyingType);
+ llvm::SmallVector<mlir::Value> newMapOpsForFields;
+ llvm::SmallVector<int64_t> fieldIdices;
+
+ for (auto fieldMemTyPair : recordType.getTypeList()) {
+ auto &field = fieldMemTyPair.first;
+ auto memTy = fieldMemTyPair.second;
+
+ bool shouldMapField =
+ llvm::find_if(mapVarForwardSlice, [&](mlir::Operation *sliceOp) {
+ if (!fir::isAllocatableType(memTy))
+ return false;
+
+ auto designateOp = mlir::dyn_cast<hlfir::DesignateOp>(sliceOp);
+ if (!designateOp)
+ return false;
+
+ return designateOp.getComponent() &&
+ designateOp.getComponent()->strref() == field;
+ }) != mapVarForwardSlice.end();
+
+ // TODO Handle recursive record types.
+
+ if (!shouldMapField)
+ continue;
+
+ int64_t fieldIdx = recordType.getFieldIndex(field);
+ bool alreadyMapped = false;
+
+ if (op.getMembersIndexAttr())
+ for (auto indexList : op.getMembersIndexAttr()) {
+ auto indexListAttr = mlir::cast<mlir::ArrayAttr>(indexList);
+ if (indexListAttr.size() == 1 &&
+ mlir::cast<mlir::IntegerAttr>(indexListAttr[0]).getInt() ==
+ fieldIdx)
+ alreadyMapped = true;
+ }
+
+ if (alreadyMapped)
+ continue;
+
+ builder.setInsertionPoint(op);
+ mlir::Value fieldIdxVal = builder.createIntegerConstant(
+ op.getLoc(), mlir::IndexType::get(builder.getContext()),
+ fieldIdx);
+ auto fieldCoord = builder.create<fir::CoordinateOp>(
+ op.getLoc(), builder.getRefType(memTy), op.getVarPtr(),
+ fieldIdxVal);
+ Fortran::lower::AddrAndBoundsInfo info =
+ Fortran::lower::getDataOperandBaseAddr(
+ builder, fieldCoord, /*isOptional=*/false, op.getLoc());
+ llvm::SmallVector<mlir::Value> bounds =
+ Fortran::lower::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
+ mlir::omp::MapBoundsType>(
+ builder, info,
+ hlfir::translateToExtendedValue(op.getLoc(), builder,
+ hlfir::Entity{fieldCoord})
+ .first,
+ /*dataExvIsAssumedSize=*/false, op.getLoc());
+
+ mlir::omp::MapInfoOp fieldMapOp =
+ builder.create<mlir::omp::MapInfoOp>(
+ op.getLoc(), fieldCoord.getResult().getType(),
+ fieldCoord.getResult(),
+ mlir::TypeAttr::get(
+ fir::unwrapRefType(fieldCoord.getResult().getType())),
+ /*varPtrPtr=*/mlir::Value{},
+ /*members=*/mlir::ValueRange{},
+ /*members_index=*/mlir::ArrayAttr{},
+ /*bounds=*/bounds, op.getMapTypeAttr(),
+ builder.getAttr<mlir::omp::VariableCaptureKindAttr>(
+ mlir::omp::VariableCaptureKind::ByRef),
+ builder.getStringAttr(op.getNameAttr().strref() + "." +
+ field + ".implicit_map"),
+ /*partial_map=*/builder.getBoolAttr(false));
+ newMapOpsForFields.emplace_back(fieldMapOp);
+ fieldIdices.emplace_back(fieldIdx);
+ }
+
+ if (!newMapOpsForFields.empty()) {
+ op.getMembersMutable().append(newMapOpsForFields);
+ llvm::SmallVector<llvm::SmallVector<int64_t>> newMemberIndices;
+ mlir::ArrayAttr oldMembersIdxAttr = op.getMembersIndexAttr();
+
+ if (oldMembersIdxAttr)
+ for (mlir::Attribute indexList : oldMembersIdxAttr) {
+ llvm::SmallVector<int64_t> listVec;
+
+ for (mlir::Attribute index :
+ mlir::cast<mlir::ArrayAttr>(indexList))
+ listVec.push_back(
+ mlir::cast<mlir::IntegerAttr>(index).getInt());
+
+ newMemberIndices.emplace_back(std::move(listVec));
+ }
+
+ for (int64_t newFieldIdx : fieldIdices) {
+ newMemberIndices.emplace_back(
+ llvm::SmallVector<int64_t>(1, newFieldIdx));
+ }
+
+ op.setMembersIndexAttr(
+ builder.create2DI64ArrayAttr(newMemberIndices));
+ }
+
+ return mlir::WalkResult::advance();
+ });
func->walk([&](mlir::omp::MapInfoOp op) {
// TODO: Currently only supports a single user for the MapInfoOp. This
diff --git a/offload/test/offloading/fortran/implicit-record-field-mapping.f90 b/offload/test/offloading/fortran/implicit-record-field-mapping.f90
new file mode 100644
index 00000000000000..8bde033d4ef4c3
--- /dev/null
+++ b/offload/test/offloading/fortran/implicit-record-field-mapping.f90
@@ -0,0 +1,63 @@
+! Test implicit mapping of alloctable record fields.
+
+! REQUIRES: flang, amdgpu
+
+! This fails only because it needs the Fortran runtime built for device. If this
+! is avaialbe, this test succeedds when run.
+! XFAIL: *
+
+! RUN: %libomptarget-compile-fortran-generic
+! RUN: env LIBOMPTARGET_INFO=16 %libomptarget-run-generic 2>&1 | %fcheck-generic
+module record_m
+ implicit none
+
+ private
+ public :: recored_t
+
+ type recored_t
+ real, allocatable :: not_to_implicitly_map(:)
+ real, allocatable :: to_implicitly_map(:)
+ end type
+
+ interface recored_t
+ end interface
+end module record_m
+
+program concurrent_inferences
+ use record_m, only : recored_t
+ implicit none
+
+ type(recored_t) :: dst_record
+ real :: src_array(10)
+ real :: dst_sum, src_sum
+ integer :: i
+
+ call random_number(src_array)
+ dst_sum = 0
+ src_sum = 0
+
+ do i=1,10
+ src_sum = src_sum + src_array(i)
+ end do
+ print *, "src_sum=", src_sum
+
+ !$omp target map(from: dst_sum)
+ dst_record%to_implicitly_map = src_array
+ dst_sum = 0
+
+ do i=1,10
+ dst_sum = dst_sum + dst_record%to_implicitly_map(i)
+ end do
+ !$omp end target
+
+ print *, "dst_sum=", dst_sum
+
+ if (src_sum == dst_sum) then
+ print *, "Test succeeded!"
+ else
+ print *, "Test failed!", " dst_sum=", dst_sum, "vs. src_sum=", src_sum
+ endif
+end program
+
+! CHECK: "PluginInterface" device {{[0-9]+}} info: Launching kernel {{.*}}
+! CHECK: Test succeeded!
|
ergawy
commented
Nov 27, 2024
Comment on lines
-624
to
-632
| // TODO: Might need revisiting to handle for non-shared clauses | ||
| if (!symAddr) { | ||
| if (const auto *details = | ||
| sym->detailsIf<Fortran::semantics::HostAssocDetails>()) { | ||
| symAddr = converter.getSymbolAddress(details->symbol()); | ||
| rawInput = symAddr; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This is a dead condition since we call symAddr.getDefiningOp() above. I think we should assert instread.
There was a problem hiding this comment.
Might be worth tagging some of the OpenACC guys to have a review of the DirectivesCommon.h changes either now or when it's in final PR form, primarily as they're the main caretakers of DirectivesCommon.h or at least the original implementors :-) I'm of course fine with the changes, but they might have some better insight than I do in this case! Although, very doubtful they'd have issues with the changeset.
agozillon
reviewed
Nov 27, 2024
There was a problem hiding this comment.
This LGTM so far, thank you very much for the work, it looks excellent so far. Left some comments here and there mainly just little notes to hopefully help out / forewarn of certain things :-)
I think the approach of doing it as an opt/transformation pass is fine personally, and I believe at least Slava from the OpenACC team originally mentioned an approach like this would be ideal for the implicit mapping. The only issue I can see someone bringing up is that we now have a split of implicit mapping between the opt passes and the initial lowering, I personally don't mind though and think it's fine, we can always move the front end lowering down to here at some point if it's a cleaner approach for the implicit mapping and it doesn't hinder anything else. And I think the opt pass approach is beneficial as we don't need to recreate the wheel for the do concurrent work, or any other work that may depend on this behavior.
Comment on lines
-624
to
-632
| // TODO: Might need revisiting to handle for non-shared clauses | ||
| if (!symAddr) { | ||
| if (const auto *details = | ||
| sym->detailsIf<Fortran::semantics::HostAssocDetails>()) { | ||
| symAddr = converter.getSymbolAddress(details->symbol()); | ||
| rawInput = symAddr; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Might be worth tagging some of the OpenACC guys to have a review of the DirectivesCommon.h changes either now or when it's in final PR form, primarily as they're the main caretakers of DirectivesCommon.h or at least the original implementors :-) I'm of course fine with the changes, but they might have some better insight than I do in this case! Although, very doubtful they'd have issues with the changeset.
| return mlir::WalkResult::advance(); | ||
|
|
||
| mlir::omp::TargetOp target = | ||
| mlir::dyn_cast_if_present<mlir::omp::TargetOp>( |
There was a problem hiding this comment.
I think this will have to work for anything that has an explicit map clause as well. From my understanding we have to map these components if someone was to write map(tofrom: some_dtype_with_allocas). Worth double checking this with @mjklemm. Hopefully isn't too hard to extend, but if it is, I'd be happy with a first pass that works for just TargetOp, which is arguably the hardest to do it for in any case :-)
|
|
||
| auto argIface = | ||
| llvm::dyn_cast<mlir::omp::BlockArgOpenMPOpInterface>(*target); | ||
| mlir::BlockArgument opBlockArg = argIface.getMapBlockArgs()[mapVarIdx]; |
There was a problem hiding this comment.
Might need to be a tad wary of how this works with use_device_addr/ptr, as they're both map info holders, and soon with Pranav's and your work I believe private may also be, and I don't think they have the same implicit connotations regular map does. We may also eventually need to be careful of this with declare mappers as well, as they're explicit user defined mappings that shouldn't have the implicit behavior but that's a problem for another day/PR when that work is further along :-)
| private | ||
| public :: recored_t | ||
|
|
||
| type recored_t |
There was a problem hiding this comment.
Excellent start! :-)
Will likely need to consider the cases where we have an allocatable derived type, with allocatable members (or regular members), and cases where we have multiple nested allocatable derived types within each other. Basically just mixes of various allocatable/pointers etc. Doesn't need to work in the first PR iteration, but at the very least we need to make sure we don't blow up the compiler or runtime mappings by accident with the new implicit mappings.
Although, I think if you can run check-offload, this should all be covered as-is, but please do make sure check-offload runs correctly for you (e.g. intentionally fail a test locally and run to make sure it does in fact test them all), there's annoying cases where check-offload doesn't actually run the fortran offload tests even when Flang and all it's relevant components have been compiled! Unsure why unfortunately, likely related to environment setup.
But still might want to prevent the generation of these in nested cases if you're going to leave them as a TODO for now, just to prevent any possible erroneous cases that aren't caught by check-offload and to prevent unnecessary mappings that may not be correct!
| mlir::Value fieldIdxVal = builder.createIntegerConstant( | ||
| op.getLoc(), mlir::IndexType::get(builder.getContext()), | ||
| fieldIdx); | ||
| auto fieldCoord = builder.create<fir::CoordinateOp>( |
There was a problem hiding this comment.
for the whole recursive generation / gather of indices, you may be able to borrow / tweak (improve on ;-)) the explicit map code that does something similar in the function: "createParentSymAndGenIntermediateMaps"
There was a problem hiding this comment.
Thanks for the pointer. Also added a todo for a later PR.
| fieldIdices.emplace_back(fieldIdx); | ||
| } | ||
|
|
||
| if (!newMapOpsForFields.empty()) { |
There was a problem hiding this comment.
It's worth noting that even implicit maps that aren't directly utilised in the TargetOp's region get inserted into the BlockArgument list of TargetOp, so please do make sure that's still occurring to maintain consistency (especially as I believe the BlockArgInterface is somewhat sensitive to it, could be incorrect however). For TargetDataOp that's not currently the case, which makes things a little weird as they'd ideally work the same way :-) Perhaps one day soon.
Would love to see a test showcasing the pre/post MapInfoFinalization pass FIR/HLFIR in the final PR when it's ready, similar to the other tests for this pass! I'll leave it up to you if you'd prefer to include it in the same test file as the rest or another, as the current test file is verifying slightly different behavior :-)
There was a problem hiding this comment.
Added an MLIR to MLIR test.
| /*members=*/mlir::ValueRange{}, | ||
| /*members_index=*/mlir::ArrayAttr{}, | ||
| /*bounds=*/bounds, op.getMapTypeAttr(), | ||
| builder.getAttr<mlir::omp::VariableCaptureKindAttr>( |
There was a problem hiding this comment.
should probably take the capture kind from the original operation as well, to keep it inline with the map type attribute, may save some possible weirdness :-)
But taking the map type from the parent does raise the interesting question of if the implicit mapping should take on the specified map type behavior of it's parent or if it should default to the regular implicit map behaviour of it's type. Not too sure on that one, the spec or one of our local specification gurus may have some insight into that.
There was a problem hiding this comment.
Maybe @mjklemm can provide some guidance here?
There was a problem hiding this comment.
Yes, I think that's the right behavior. However, we need to bear in mind that a user might have used declare mapper to define a mapper for a derived type. In that case, we might need to honor the user-defined mapper instead.
ergawy
force-pushed
the
implicit_record_field_mapping
branch
3 times, most recently
from
e607bc8 to
12c34e6
Compare
ergawy
changed the title
ergawy
force-pushed
the
implicit_record_field_mapping
branch
from
12c34e6 to
298c1fb
Compare
ergawy
requested review from
clementval,
skatrak,
mjklemm,
agozillon,
TIFitis and
vzakhari
ergawy
force-pushed
the
implicit_record_field_mapping
branch
from
298c1fb to
7fd5fc9
Compare
|
Ping 🔔! Would be nice to take a look when you folks have a chance 🙏! There is still quite some work to here in follow up PRs. |
luporl
reviewed
Dec 3, 2024
flang/test/Transforms/omp-map-info-finalization-implicit-field.fir
Outdated
Show resolved
Hide resolved
offload/test/offloading/fortran/implicit-record-field-mapping.f90
Outdated
Show resolved
Hide resolved
ergawy
force-pushed
the
implicit_record_field_mapping
branch
2 times, most recently
from
e45ea4a to
7d4e732
Compare
agozillon
approved these changes
Dec 10, 2024
ergawy
force-pushed
the
implicit_record_field_mapping
branch
from
7d4e732 to
684817e
Compare
This is a starting PR to implicitly map allocatable record fields. This PR contains the following changes: 1. Re-purposes some of the utils used in `Lower/OpenMP.cpp` so that these utils work on the `mlir::Value` level rather than the `semantics::Symbol` level. This takes one step towards to enabling MLIR passes to more easily do some lowering themselves (e.g. creating `omp.map.bounds` ops for implicitely caputured data like this PR does). 2. Adds support for implicitely capturing and mapping allocatable fields in record types. There is quite some distant to still cover to have full support for this. I added a number of todos to guide further development. Co-authored-by: Andrew Gozillon <[email protected]>
ergawy
force-pushed
the
implicit_record_field_mapping
branch
from
684817e to
8af3b52
Compare
|
Ping 🔔! Please let me know if there are objections to merging this PR. Since a lot of people are on vacation, I am waiting a bit more before merging to give more time. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/89/builds/12915 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/157/builds/15551 Here is the relevant piece of the build log for the reference |
|
@ergawy I'm seeing the same error as the buildbots above. Would you mind taking a look? Thanks! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/138/builds/7913 Here is the relevant piece of the build log for the reference |
|
@kazutakahirata Yes, doing that now ... |
ergawy
added a commit
to ergawy/llvm-project
that referenced
this pull request
Dec 18, 2024
…m#117867)" Reverting due to build errors which I am looking into at the moment. Just reverting to not disrupt anyone while investigating the issue. See: llvm#117867 (comment) This reverts commit db09014.
|
Created a revert PR for now: #120360 while trying to reproduce the issue locally. |
ergawy
added a commit
that referenced
this pull request
Dec 18, 2024
ergawy
added a commit
to ergawy/llvm-project
that referenced
this pull request
Dec 18, 2024
…cord fields" This re-applies llvm#117867 with a small fix that hopefully prevents build bot failures. The fix is avoiding `dyn_cast` for the result of `getOperation()`. Instead we can assign the result to `mlir::ModuleOp` directly since the type of the operation is known statically (`OpT` in `OperationPass`).
ergawy
added a commit
that referenced
this pull request
Dec 18, 2024
… fields (#120374) This re-applies #117867 with a small fix that hopefully prevents build bot failures. The fix is avoiding `dyn_cast` for the result of `getOperation()`. Instead we can assign the result to `mlir::ModuleOp` directly since the type of the operation is known statically (`OpT` in `OperationPass`).
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 is a starting PR to implicitly map allocatable record fields.
This PR contains the following changes:
Lower/OpenMP.cppso thatthese utils work on the
mlir::Valuelevel rather than thesemantics::Symbollevel. This takes one step towards to enablingMLIR passes to more easily do some lowering themselves (e.g. creating
omp.map.boundsops for implicitely caputured data like this PRdoes).
in record types.
There is quite some distant to still cover to have full support for
this. I added a number of todos to guide further development.
Co-authored-by: Andrew Gozillon [email protected]