[Flang][OpenMP][MLIR] Fix common block mapping for regular and declare target link #91829
Conversation
agozillon
requested review from
skatrak,
jsjodin,
raghavendhra,
DominikAdamski,
dpalermo,
TIFitis and
kparzysz
llvmbot
added
mlir:llvm
mlir
flang
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-mlir Author: None (agozillon) ChangesThis PR attempts to fix common block mapping for regular mapping of these types as well as when they have been marked as "declare target link". This PR should allow correct mapping of both the members of a common block and the full common block via its block symbol. The main changes were some adjustments to the Fortran OpenMP lowering to HLFIR/FIR, the lowering of the LLVM+OpenMP dialect to LLVM-IR and adjustments to the way the we handle target kernel map argument rebinding inside of the OMPIRBuilder. For the Fortran OpenMP lowering were two changes, one to prevent the implicit capture of common block members when the common block symbol itself has been marked and the other creates intermediate member access inside of the target region to be used in-place of those external to the target region, this prevents external usages breaking the IsolatedFromAbove pact. In the latter case, there was an adjustment to the size calculation for types to better handle cases where we pass an array as the type of a map (as opposed to the bounds and the type of the element), which occurs in the case of common blocks. There is also some adjustment to how handleDeclareTargetMapVar handles renaming of declare target symbols in the module to the reference pointer, now it will only apply to those within the kernel that is currently being generated and we also perform a modification to replace constants with instructions as necessary as we cannot replace these with our reference pointer (non-constant and constants do not mix nicely). In the case of the OpenMPIRBuilder some changes were made to defer global symbol rebinding to kernel arguments until all other arguments have been rebound. This makes sure we do not replace uses that may refer to the global (e.g. a GEP) but are themselves actually a separate argument that needs bound. Currently "declare target to" still needs some work, but this may be the case for all types in conjunction with "declare target to" at the moment. Patch is 33.95 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/91829.diff 12 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index f23902d6a8239..07a8535d7bc6a 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -830,6 +830,33 @@ static void genBodyOfTargetDataOp(
genNestedEvaluations(converter, eval);
}
+// This generates intermediate common block member accesses within a region
+// and then rebinds the members symbol to the intermediate accessors we have
+// generated so that subsequent code generation will utilise these instead.
+//
+// When the scope changes, the bindings to the intermediate accessors should
+// be dropped in place of the original symbol bindings.
+//
+// This is for utilisation with TargetOp.
+static void genIntermediateCommonBlockAccessors(
+ Fortran::lower::AbstractConverter &converter,
+ const mlir::Location ¤tLocation, mlir::Region ®ion,
+ llvm::ArrayRef<const Fortran::semantics::Symbol *> mapSyms) {
+ for (auto [argIndex, argSymbol] : llvm::enumerate(mapSyms)) {
+ if (auto *details =
+ argSymbol->detailsIf<Fortran::semantics::CommonBlockDetails>()) {
+ for (auto obj : details->objects()) {
+ auto targetCBMemberBind = Fortran::lower::genCommonBlockMember(
+ converter, currentLocation, *obj, region.getArgument(argIndex));
+ fir::ExtendedValue sexv = converter.getSymbolExtendedValue(*obj);
+ fir::ExtendedValue targetCBExv =
+ getExtendedValue(sexv, targetCBMemberBind);
+ converter.bindSymbol(*obj, targetCBExv);
+ }
+ }
+ }
+}
+
// This functions creates a block for the body of the targetOp's region. It adds
// all the symbols present in mapSymbols as block arguments to this block.
static void
@@ -983,6 +1010,18 @@ genBodyOfTargetOp(Fortran::lower::AbstractConverter &converter,
// 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 it's members within the target region, there is a large
+ // chance we will end up with uses external to the region accessing the common
+ // block. As target regions are IsolatedFromAbove, we must make sure to
+ // 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, region,
+ mapSyms);
+
if (genNested)
genNestedEvaluations(converter, eval);
}
@@ -1574,6 +1613,13 @@ genTargetOp(Fortran::lower::AbstractConverter &converter,
// symbols used inside the region that have not been explicitly mapped using
// the map clause.
auto captureImplicitMap = [&](const Fortran::semantics::Symbol &sym) {
+ // if the symbol is part of an already mapped common block, do not make a
+ // map for it.
+ if (const Fortran::semantics::Symbol *common =
+ Fortran::semantics::FindCommonBlockContaining(sym.GetUltimate()))
+ if (llvm::find(mapSyms, common) != mapSyms.end())
+ return;
+
if (llvm::find(mapSyms, &sym) == mapSyms.end()) {
mlir::Value baseOp = converter.getSymbolAddress(sym);
if (!baseOp)
diff --git a/flang/test/Integration/OpenMP/map-types-and-sizes.f90 b/flang/test/Integration/OpenMP/map-types-and-sizes.f90
index f3a20690f05a9..591be0b680a51 100644
--- a/flang/test/Integration/OpenMP/map-types-and-sizes.f90
+++ b/flang/test/Integration/OpenMP/map-types-and-sizes.f90
@@ -231,6 +231,31 @@ subroutine mapType_char
!$omp end target
end subroutine mapType_char
+!CHECK: @.offload_sizes{{.*}} = private unnamed_addr constant [1 x i64] [i64 8]
+!CHECK: @.offload_maptypes{{.*}} = private unnamed_addr constant [1 x i64] [i64 35]
+subroutine mapType_common_block
+ implicit none
+ common /var_common/ var1, var2
+ integer :: var1, var2
+!$omp target map(tofrom: /var_common/)
+ var1 = var1 + 20
+ var2 = var2 + 30
+!$omp end target
+end subroutine mapType_common_block
+
+!CHECK: @.offload_sizes{{.*}} = private unnamed_addr constant [2 x i64] [i64 4, i64 4]
+!CHECK: @.offload_maptypes{{.*}} = private unnamed_addr constant [2 x i64] [i64 35, i64 35]
+subroutine mapType_common_block_members
+ implicit none
+ common /var_common/ var1, var2
+ integer :: var1, var2
+
+!$omp target map(tofrom: var1, var2)
+ var2 = var1
+!$omp end target
+end subroutine mapType_common_block_members
+
+
!CHECK-LABEL: define {{.*}} @{{.*}}maptype_ptr_explicit_{{.*}}
!CHECK: %[[ALLOCA:.*]] = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
!CHECK: %[[ALLOCA_GEP:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8 }, ptr %[[ALLOCA]], i32 1
@@ -346,3 +371,19 @@ end subroutine mapType_char
!CHECK: store ptr %[[ALLOCA]], ptr %[[BASE_PTR_ARR]], align 8
!CHECK: %[[OFFLOAD_PTR_ARR:.*]] = getelementptr inbounds [1 x ptr], ptr %.offload_ptrs, i32 0, i32 0
!CHECK: store ptr %[[ARR_OFF]], ptr %[[OFFLOAD_PTR_ARR]], align 8
+
+!CHECK-LABEL: define {{.*}} @{{.*}}maptype_common_block_{{.*}}
+!CHECK: %[[BASE_PTR_ARR:.*]] = getelementptr inbounds [1 x ptr], ptr %.offload_baseptrs, i32 0, i32 0
+!CHECK: store ptr @var_common_, ptr %[[BASE_PTR_ARR]], align 8
+!CHECK: %[[OFFLOAD_PTR_ARR:.*]] = getelementptr inbounds [1 x ptr], ptr %.offload_ptrs, i32 0, i32 0
+!CHECK: store ptr @var_common_, ptr %[[OFFLOAD_PTR_ARR]], align 8
+
+!CHECK-LABEL: define {{.*}} @{{.*}}maptype_common_block_members_{{.*}}
+!CHECK: %[[BASE_PTR_ARR:.*]] = getelementptr inbounds [2 x ptr], ptr %.offload_baseptrs, i32 0, i32 0
+!CHECK: store ptr @var_common_, ptr %[[BASE_PTR_ARR]], align 8
+!CHECK: %[[OFFLOAD_PTR_ARR:.*]] = getelementptr inbounds [2 x ptr], ptr %.offload_ptrs, i32 0, i32 0
+!CHECK: store ptr @var_common_, ptr %[[OFFLOAD_PTR_ARR]], align 8
+!CHECK: %[[BASE_PTR_ARR_1:.*]] = getelementptr inbounds [2 x ptr], ptr %.offload_baseptrs, i32 0, i32 1
+!CHECK: store ptr getelementptr (i8, ptr @var_common_, i64 4), ptr %[[BASE_PTR_ARR_1]], align 8
+!CHECK: %[[OFFLOAD_PTR_ARR_1:.*]] = getelementptr inbounds [2 x ptr], ptr %.offload_ptrs, i32 0, i32 1
+!CHECK: store ptr getelementptr (i8, ptr @var_common_, i64 4), ptr %[[OFFLOAD_PTR_ARR_1]], align 8
diff --git a/flang/test/Lower/OpenMP/common-block-map.f90 b/flang/test/Lower/OpenMP/common-block-map.f90
new file mode 100644
index 0000000000000..56a5e775b1b65
--- /dev/null
+++ b/flang/test/Lower/OpenMP/common-block-map.f90
@@ -0,0 +1,83 @@
+!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+
+!CHECK: fir.global common @var_common_(dense<0> : vector<8xi8>) : !fir.array<8xi8>
+!CHECK: fir.global common @var_common_link_(dense<0> : vector<8xi8>) {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (link)>} : !fir.array<8xi8>
+
+!CHECK-LABEL: func.func @_QPmap_full_block
+!CHECK: %[[CB_ADDR:.*]] = fir.address_of(@var_common_) : !fir.ref<!fir.array<8xi8>>
+!CHECK: %[[MAP:.*]] = omp.map.info var_ptr(%[[CB_ADDR]] : !fir.ref<!fir.array<8xi8>>, !fir.array<8xi8>) map_clauses(tofrom) capture(ByRef) -> !fir.ref<!fir.array<8xi8>> {name = "var_common"}
+!CHECK: omp.target map_entries(%[[MAP]] -> %[[MAP_ARG:.*]] : !fir.ref<!fir.array<8xi8>>) {
+!CHECK: ^bb0(%[[MAP_ARG]]: !fir.ref<!fir.array<8xi8>>):
+!CHECK: %[[CONV:.*]] = fir.convert %[[MAP_ARG]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK: %[[INDEX:.*]] = arith.constant 0 : index
+!CHECK: %[[COORD:.*]] = fir.coordinate_of %[[CONV]], %[[INDEX]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK: %[[CONV2:.*]] = fir.convert %[[COORD]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK: %[[CB_MEMBER_1:.*]]:2 = hlfir.declare %[[CONV2]] {uniq_name = "_QFmap_full_blockEvar1"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK: %[[CONV3:.*]] = fir.convert %[[MAP_ARG]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK: %[[INDEX2:.*]] = arith.constant 4 : index
+!CHECK: %[[COORD2:.*]] = fir.coordinate_of %[[CONV3]], %[[INDEX2]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK: %[[CONV4:.*]] = fir.convert %[[COORD2]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK: %[[CB_MEMBER_2:.*]]:2 = hlfir.declare %[[CONV4]] {uniq_name = "_QFmap_full_blockEvar2"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+subroutine map_full_block
+ implicit none
+ common /var_common/ var1, var2
+ integer :: var1, var2
+!$omp target map(tofrom: /var_common/)
+ var1 = var1 + 20
+ var2 = var2 + 30
+!$omp end target
+end
+
+!CHECK-LABEL: @_QPmap_mix_of_members
+!CHECK: %[[COMMON_BLOCK:.*]] = fir.address_of(@var_common_) : !fir.ref<!fir.array<8xi8>>
+!CHECK: %[[CB_CONV:.*]] = fir.convert %[[COMMON_BLOCK]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK: %[[INDEX:.*]] = arith.constant 0 : index
+!CHECK: %[[COORD:.*]] = fir.coordinate_of %[[CB_CONV]], %[[INDEX]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK: %[[CONV:.*]] = fir.convert %[[COORD]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK: %[[CB_MEMBER_1:.*]]:2 = hlfir.declare %[[CONV]] {uniq_name = "_QFmap_mix_of_membersEvar1"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK: %[[CB_CONV:.*]] = fir.convert %[[COMMON_BLOCK]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK: %[[INDEX:.*]] = arith.constant 4 : index
+!CHECK: %[[COORD:.*]] = fir.coordinate_of %[[CB_CONV]], %[[INDEX]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK: %[[CONV:.*]] = fir.convert %[[COORD]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK: %[[CB_MEMBER_2:.*]]:2 = hlfir.declare %[[CONV]] {uniq_name = "_QFmap_mix_of_membersEvar2"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK: %[[MAP_EXP:.*]] = omp.map.info var_ptr(%[[CB_MEMBER_2]]#0 : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "var2"}
+!CHECK: %[[MAP_IMP:.*]] = omp.map.info var_ptr(%[[CB_MEMBER_1]]#1 : !fir.ref<i32>, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref<i32> {name = "var1"}
+!CHECK: omp.target map_entries(%[[MAP_EXP]] -> %[[ARG_EXP:.*]], %[[MAP_IMP]] -> %[[ARG_IMP:.*]] : !fir.ref<i32>, !fir.ref<i32>) {
+!CHECK: ^bb0(%[[ARG_EXP]]: !fir.ref<i32>, %[[ARG_IMP]]: !fir.ref<i32>):
+!CHECK: %[[EXP_MEMBER:.*]]:2 = hlfir.declare %[[ARG_EXP]] {uniq_name = "_QFmap_mix_of_membersEvar2"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK: %[[IMP_MEMBER:.*]]:2 = hlfir.declare %[[ARG_IMP]] {uniq_name = "_QFmap_mix_of_membersEvar1"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+subroutine map_mix_of_members
+ implicit none
+ common /var_common/ var1, var2
+ integer :: var1, var2
+
+!$omp target map(tofrom: var2)
+ var2 = var1
+!$omp end target
+end
+
+!CHECK-LABEL: @_QQmain
+!CHECK: %[[DECL_TAR_CB:.*]] = fir.address_of(@var_common_link_) : !fir.ref<!fir.array<8xi8>>
+!CHECK: %[[MAP_DECL_TAR_CB:.*]] = omp.map.info var_ptr(%[[DECL_TAR_CB]] : !fir.ref<!fir.array<8xi8>>, !fir.array<8xi8>) map_clauses(tofrom) capture(ByRef) -> !fir.ref<!fir.array<8xi8>> {name = "var_common_link"}
+!CHECK: omp.target map_entries(%[[MAP_DECL_TAR_CB]] -> %[[MAP_DECL_TAR_ARG:.*]] : !fir.ref<!fir.array<8xi8>>) {
+!CHECK: ^bb0(%[[MAP_DECL_TAR_ARG]]: !fir.ref<!fir.array<8xi8>>):
+!CHECK: %[[CONV:.*]] = fir.convert %[[MAP_DECL_TAR_ARG]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK: %[[INDEX:.*]] = arith.constant 0 : index
+!CHECK: %[[COORD:.*]] = fir.coordinate_of %[[CONV]], %[[INDEX]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK: %[[CONV:.*]] = fir.convert %[[COORD]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK: %[[MEMBER_ONE:.*]]:2 = hlfir.declare %[[CONV]] {uniq_name = "_QFElink1"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK: %[[CONV:.*]] = fir.convert %[[MAP_DECL_TAR_ARG]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK: %[[INDEX:.*]] = arith.constant 4 : index
+!CHECK: %[[COORD:.*]] = fir.coordinate_of %[[CONV]], %[[INDEX]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK: %[[CONV:.*]] = fir.convert %[[COORD]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK: %[[MEMBER_TWO:.*]]:2 = hlfir.declare %[[CONV]] {uniq_name = "_QFElink2"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+program main
+ implicit none
+ common /var_common_link/ link1, link2
+ integer :: link1, link2
+ !$omp declare target link(/var_common_link/)
+
+!$omp target map(tofrom: /var_common_link/)
+ link1 = link2 + 20
+!$omp end target
+end program
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h b/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
index c9ee0c25194c2..0f078b03457b0 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
+++ b/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
@@ -2110,6 +2110,11 @@ class OpenMPIRBuilder {
int32_t UB);
///}
+ /// Replaces constant values with instruction equivelants where possible
+ /// inside of a function.
+ static void replaceConstantValueUsesInFuncWithInstr(llvm::Value *Input,
+ Function *Func);
+
private:
// Sets the function attributes expected for the outlined function
void setOutlinedTargetRegionFunctionAttributes(Function *OutlinedFn);
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 42ea20919a5e1..4d803e65ae542 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -5085,8 +5085,8 @@ static void replaceConstatExprUsesInFuncWithInstr(ConstantExpr *ConstExpr,
}
}
-static void replaceConstantValueUsesInFuncWithInstr(llvm::Value *Input,
- Function *Func) {
+void OpenMPIRBuilder::replaceConstantValueUsesInFuncWithInstr(
+ llvm::Value *Input, Function *Func) {
for (User *User : make_early_inc_range(Input->users()))
if (auto *Const = dyn_cast<Constant>(User))
if (auto *ConstExpr = dyn_cast<ConstantExpr>(Const))
@@ -5160,6 +5160,31 @@ static Function *createOutlinedFunction(
? make_range(Func->arg_begin() + 1, Func->arg_end())
: Func->args();
+ auto ReplaceValue = [&OMPBuilder](Value *Input, Value *InputCopy,
+ Function *Func) {
+ // Things like GEP's can come in the form of Constants. Constants and
+ // ConstantExpr's do not have access to the knowledge of what they're
+ // contained in, so we must dig a little to find an instruction so we
+ // can tell if they're used inside of the function we're outlining. We
+ // also replace the original constant expression with a new instruction
+ // equivalent; an instruction as it allows easy modification in the
+ // following loop, as we can now know the constant (instruction) is
+ // owned by our target function and replaceUsesOfWith can now be invoked
+ // on it (cannot do this with constants it seems). A brand new one also
+ // allows us to be cautious as it is perhaps possible the old expression
+ // was used inside of the function but exists and is used externally
+ // (unlikely by the nature of a Constant, but still).
+ OMPBuilder.replaceConstantValueUsesInFuncWithInstr(Input, Func);
+
+ // Collect all the instructions
+ for (User *User : make_early_inc_range(Input->users()))
+ if (auto *Instr = dyn_cast<Instruction>(User))
+ if (Instr->getFunction() == Func)
+ Instr->replaceUsesOfWith(Input, InputCopy);
+ };
+
+ SmallVector<std::pair<Value *, Value *>> DeferredReplacement;
+
// Rewrite uses of input valus to parameters.
for (auto InArg : zip(Inputs, ArgRange)) {
Value *Input = std::get<0>(InArg);
@@ -5169,27 +5194,36 @@ static Function *createOutlinedFunction(
Builder.restoreIP(
ArgAccessorFuncCB(Arg, Input, InputCopy, AllocaIP, Builder.saveIP()));
- // Things like GEP's can come in the form of Constants. Constants and
- // ConstantExpr's do not have access to the knowledge of what they're
- // contained in, so we must dig a little to find an instruction so we can
- // tell if they're used inside of the function we're outlining. We also
- // replace the original constant expression with a new instruction
- // equivalent; an instruction as it allows easy modification in the
- // following loop, as we can now know the constant (instruction) is owned by
- // our target function and replaceUsesOfWith can now be invoked on it
- // (cannot do this with constants it seems). A brand new one also allows us
- // to be cautious as it is perhaps possible the old expression was used
- // inside of the function but exists and is used externally (unlikely by the
- // nature of a Constant, but still).
- replaceConstantValueUsesInFuncWithInstr(Input, Func);
+ // In certain cases a Global may be set up for replacement, however, this
+ // Global may be used in multiple arguments to the kernel, just segmented
+ // apart, for example, if we have a global array, that is sectioned into
+ // multiple mappings (technically not legal in OpenMP, but there is a case
+ // in Fortran for Common Blocks where this is neccesary), we will end up
+ // with GEP's into this array inside the kernel, that refer to the Global
+ // but are technically seperate arguments to the kernel for all intents and
+ // purposes. If we have mapped a segment that requires a GEP into the 0-th
+ // index, it will fold into an referal to the Global, if we then encounter
+ // this folded GEP during replacement all of the references to the
+ // Global in the kernel will be replaced with the argument we have generated
+ // that corresponds to it, including any other GEP's that refer to the
+ // Global that may be other arguments. This will invalidate all of the other
+ // preceding mapped arguments that refer to the same global that may be
+ // seperate segments. To prevent this, we defer global processing until all
+ // other processing has been performed.
+ if (llvm::isa<llvm::GlobalValue>(std::get<0>(InArg)) ||
+ llvm::isa<llvm::GlobalObject>(std::get<0>(InArg)) ||
+ llvm::isa<llvm::GlobalVariable>(std::get<0>(InArg))) {
+ DeferredReplacement.push_back(std::make_pair(Input, InputCopy));
+ continue;
+ }
- // Collect all the instructions
- for (User *User : make_early_inc_range(Input->users()))
- if (auto *Instr = dyn_cast<Instruction>(User))
- if (Instr->getFunction() == Func)
- Instr->replaceUsesOfWith(Input, InputCopy);
+ ReplaceValue(Input, InputCopy, Func);
}
+ // Replace all of our deferred Input values, currently just Globals.
+ for (auto Deferred : DeferredReplacement)
+ ReplaceValue(std::get<0>(Deferred), std::get<1>(Deferred), Func);
+
// Restore insert point.
Builder.restoreIP(OldInsertPoint);
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 0c34126667324..d2b4d77cfdcb8 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -2021,12 +2021,6 @@ llvm::Value *getSizeInBytes(DataLayout &dl, const mlir::Type &type,
Operation *clauseOp, llvm::Value *basePointer,
llvm::Type *baseType, llvm::IRBuilderBase &builder,
LLVM::ModuleTranslation &moduleTranslation) {
- // utilising getTypeSizeInBits instead of getTypeSize as getTypeSize gives
- // the size in inconsistent byte or bit format.
- uint64_t underlyingTypeSzInBits = dl.getTypeSizeInBits(type);
- if (auto arrTy = llvm::dyn_cast_if_present<LLVM::LLVMArrayType>(type))
- underlyingTypeSzInBits = getArrayElementSizeInBits(arrTy, dl);
-
if (auto memberClause =
mlir::dyn_cast_if_present<mlir::omp::MapInfoOp>(clauseOp)) {
// This calculates the size to transfer based on bounds and the underlying
@@ -2052,6 +2046,12 @@ llvm::Value *getSizeInBytes(DataLayout &dl, const mlir::Type &type,
}
}
+ // utilising getTy...
[truncated]
|
|
@llvm/pr-subscribers-mlir-openmp Author: None (agozillon) ChangesThis PR attempts to fix common block mapping for regular mapping of these types as well as when they have been marked as "declare target link". This PR should allow correct mapping of both the members of a common block and the full common block via its block symbol. The main changes were some adjustments to the Fortran OpenMP lowering to HLFIR/FIR, the lowering of the LLVM+OpenMP dialect to LLVM-IR and adjustments to the way the we handle target kernel map argument rebinding inside of the OMPIRBuilder. For the Fortran OpenMP lowering were two changes, one to prevent the implicit capture of common block members when the common block symbol itself has been marked and the other creates intermediate member access inside of the target region to be used in-place of those external to the target region, this prevents external usages breaking the IsolatedFromAbove pact. In the latter case, there was an adjustment to the size calculation for types to better handle cases where we pass an array as the type of a map (as opposed to the bounds and the type of the element), which occurs in the case of common blocks. There is also some adjustment to how handleDeclareTargetMapVar handles renaming of declare target symbols in the module to the reference pointer, now it will only apply to those within the kernel that is currently being generated and we also perform a modification to replace constants with instructions as necessary as we cannot replace these with our reference pointer (non-constant and constants do not mix nicely). In the case of the OpenMPIRBuilder some changes were made to defer global symbol rebinding to kernel arguments until all other arguments have been rebound. This makes sure we do not replace uses that may refer to the global (e.g. a GEP) but are themselves actually a separate argument that needs bound. Currently "declare target to" still needs some work, but this may be the case for all types in conjunction with "declare target to" at the moment. Patch is 33.95 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/91829.diff 12 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index f23902d6a8239..07a8535d7bc6a 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -830,6 +830,33 @@ static void genBodyOfTargetDataOp(
genNestedEvaluations(converter, eval);
}
+// This generates intermediate common block member accesses within a region
+// and then rebinds the members symbol to the intermediate accessors we have
+// generated so that subsequent code generation will utilise these instead.
+//
+// When the scope changes, the bindings to the intermediate accessors should
+// be dropped in place of the original symbol bindings.
+//
+// This is for utilisation with TargetOp.
+static void genIntermediateCommonBlockAccessors(
+ Fortran::lower::AbstractConverter &converter,
+ const mlir::Location ¤tLocation, mlir::Region ®ion,
+ llvm::ArrayRef<const Fortran::semantics::Symbol *> mapSyms) {
+ for (auto [argIndex, argSymbol] : llvm::enumerate(mapSyms)) {
+ if (auto *details =
+ argSymbol->detailsIf<Fortran::semantics::CommonBlockDetails>()) {
+ for (auto obj : details->objects()) {
+ auto targetCBMemberBind = Fortran::lower::genCommonBlockMember(
+ converter, currentLocation, *obj, region.getArgument(argIndex));
+ fir::ExtendedValue sexv = converter.getSymbolExtendedValue(*obj);
+ fir::ExtendedValue targetCBExv =
+ getExtendedValue(sexv, targetCBMemberBind);
+ converter.bindSymbol(*obj, targetCBExv);
+ }
+ }
+ }
+}
+
// This functions creates a block for the body of the targetOp's region. It adds
// all the symbols present in mapSymbols as block arguments to this block.
static void
@@ -983,6 +1010,18 @@ genBodyOfTargetOp(Fortran::lower::AbstractConverter &converter,
// 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 it's members within the target region, there is a large
+ // chance we will end up with uses external to the region accessing the common
+ // block. As target regions are IsolatedFromAbove, we must make sure to
+ // 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, region,
+ mapSyms);
+
if (genNested)
genNestedEvaluations(converter, eval);
}
@@ -1574,6 +1613,13 @@ genTargetOp(Fortran::lower::AbstractConverter &converter,
// symbols used inside the region that have not been explicitly mapped using
// the map clause.
auto captureImplicitMap = [&](const Fortran::semantics::Symbol &sym) {
+ // if the symbol is part of an already mapped common block, do not make a
+ // map for it.
+ if (const Fortran::semantics::Symbol *common =
+ Fortran::semantics::FindCommonBlockContaining(sym.GetUltimate()))
+ if (llvm::find(mapSyms, common) != mapSyms.end())
+ return;
+
if (llvm::find(mapSyms, &sym) == mapSyms.end()) {
mlir::Value baseOp = converter.getSymbolAddress(sym);
if (!baseOp)
diff --git a/flang/test/Integration/OpenMP/map-types-and-sizes.f90 b/flang/test/Integration/OpenMP/map-types-and-sizes.f90
index f3a20690f05a9..591be0b680a51 100644
--- a/flang/test/Integration/OpenMP/map-types-and-sizes.f90
+++ b/flang/test/Integration/OpenMP/map-types-and-sizes.f90
@@ -231,6 +231,31 @@ subroutine mapType_char
!$omp end target
end subroutine mapType_char
+!CHECK: @.offload_sizes{{.*}} = private unnamed_addr constant [1 x i64] [i64 8]
+!CHECK: @.offload_maptypes{{.*}} = private unnamed_addr constant [1 x i64] [i64 35]
+subroutine mapType_common_block
+ implicit none
+ common /var_common/ var1, var2
+ integer :: var1, var2
+!$omp target map(tofrom: /var_common/)
+ var1 = var1 + 20
+ var2 = var2 + 30
+!$omp end target
+end subroutine mapType_common_block
+
+!CHECK: @.offload_sizes{{.*}} = private unnamed_addr constant [2 x i64] [i64 4, i64 4]
+!CHECK: @.offload_maptypes{{.*}} = private unnamed_addr constant [2 x i64] [i64 35, i64 35]
+subroutine mapType_common_block_members
+ implicit none
+ common /var_common/ var1, var2
+ integer :: var1, var2
+
+!$omp target map(tofrom: var1, var2)
+ var2 = var1
+!$omp end target
+end subroutine mapType_common_block_members
+
+
!CHECK-LABEL: define {{.*}} @{{.*}}maptype_ptr_explicit_{{.*}}
!CHECK: %[[ALLOCA:.*]] = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
!CHECK: %[[ALLOCA_GEP:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8 }, ptr %[[ALLOCA]], i32 1
@@ -346,3 +371,19 @@ end subroutine mapType_char
!CHECK: store ptr %[[ALLOCA]], ptr %[[BASE_PTR_ARR]], align 8
!CHECK: %[[OFFLOAD_PTR_ARR:.*]] = getelementptr inbounds [1 x ptr], ptr %.offload_ptrs, i32 0, i32 0
!CHECK: store ptr %[[ARR_OFF]], ptr %[[OFFLOAD_PTR_ARR]], align 8
+
+!CHECK-LABEL: define {{.*}} @{{.*}}maptype_common_block_{{.*}}
+!CHECK: %[[BASE_PTR_ARR:.*]] = getelementptr inbounds [1 x ptr], ptr %.offload_baseptrs, i32 0, i32 0
+!CHECK: store ptr @var_common_, ptr %[[BASE_PTR_ARR]], align 8
+!CHECK: %[[OFFLOAD_PTR_ARR:.*]] = getelementptr inbounds [1 x ptr], ptr %.offload_ptrs, i32 0, i32 0
+!CHECK: store ptr @var_common_, ptr %[[OFFLOAD_PTR_ARR]], align 8
+
+!CHECK-LABEL: define {{.*}} @{{.*}}maptype_common_block_members_{{.*}}
+!CHECK: %[[BASE_PTR_ARR:.*]] = getelementptr inbounds [2 x ptr], ptr %.offload_baseptrs, i32 0, i32 0
+!CHECK: store ptr @var_common_, ptr %[[BASE_PTR_ARR]], align 8
+!CHECK: %[[OFFLOAD_PTR_ARR:.*]] = getelementptr inbounds [2 x ptr], ptr %.offload_ptrs, i32 0, i32 0
+!CHECK: store ptr @var_common_, ptr %[[OFFLOAD_PTR_ARR]], align 8
+!CHECK: %[[BASE_PTR_ARR_1:.*]] = getelementptr inbounds [2 x ptr], ptr %.offload_baseptrs, i32 0, i32 1
+!CHECK: store ptr getelementptr (i8, ptr @var_common_, i64 4), ptr %[[BASE_PTR_ARR_1]], align 8
+!CHECK: %[[OFFLOAD_PTR_ARR_1:.*]] = getelementptr inbounds [2 x ptr], ptr %.offload_ptrs, i32 0, i32 1
+!CHECK: store ptr getelementptr (i8, ptr @var_common_, i64 4), ptr %[[OFFLOAD_PTR_ARR_1]], align 8
diff --git a/flang/test/Lower/OpenMP/common-block-map.f90 b/flang/test/Lower/OpenMP/common-block-map.f90
new file mode 100644
index 0000000000000..56a5e775b1b65
--- /dev/null
+++ b/flang/test/Lower/OpenMP/common-block-map.f90
@@ -0,0 +1,83 @@
+!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+
+!CHECK: fir.global common @var_common_(dense<0> : vector<8xi8>) : !fir.array<8xi8>
+!CHECK: fir.global common @var_common_link_(dense<0> : vector<8xi8>) {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (link)>} : !fir.array<8xi8>
+
+!CHECK-LABEL: func.func @_QPmap_full_block
+!CHECK: %[[CB_ADDR:.*]] = fir.address_of(@var_common_) : !fir.ref<!fir.array<8xi8>>
+!CHECK: %[[MAP:.*]] = omp.map.info var_ptr(%[[CB_ADDR]] : !fir.ref<!fir.array<8xi8>>, !fir.array<8xi8>) map_clauses(tofrom) capture(ByRef) -> !fir.ref<!fir.array<8xi8>> {name = "var_common"}
+!CHECK: omp.target map_entries(%[[MAP]] -> %[[MAP_ARG:.*]] : !fir.ref<!fir.array<8xi8>>) {
+!CHECK: ^bb0(%[[MAP_ARG]]: !fir.ref<!fir.array<8xi8>>):
+!CHECK: %[[CONV:.*]] = fir.convert %[[MAP_ARG]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK: %[[INDEX:.*]] = arith.constant 0 : index
+!CHECK: %[[COORD:.*]] = fir.coordinate_of %[[CONV]], %[[INDEX]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK: %[[CONV2:.*]] = fir.convert %[[COORD]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK: %[[CB_MEMBER_1:.*]]:2 = hlfir.declare %[[CONV2]] {uniq_name = "_QFmap_full_blockEvar1"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK: %[[CONV3:.*]] = fir.convert %[[MAP_ARG]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK: %[[INDEX2:.*]] = arith.constant 4 : index
+!CHECK: %[[COORD2:.*]] = fir.coordinate_of %[[CONV3]], %[[INDEX2]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK: %[[CONV4:.*]] = fir.convert %[[COORD2]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK: %[[CB_MEMBER_2:.*]]:2 = hlfir.declare %[[CONV4]] {uniq_name = "_QFmap_full_blockEvar2"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+subroutine map_full_block
+ implicit none
+ common /var_common/ var1, var2
+ integer :: var1, var2
+!$omp target map(tofrom: /var_common/)
+ var1 = var1 + 20
+ var2 = var2 + 30
+!$omp end target
+end
+
+!CHECK-LABEL: @_QPmap_mix_of_members
+!CHECK: %[[COMMON_BLOCK:.*]] = fir.address_of(@var_common_) : !fir.ref<!fir.array<8xi8>>
+!CHECK: %[[CB_CONV:.*]] = fir.convert %[[COMMON_BLOCK]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK: %[[INDEX:.*]] = arith.constant 0 : index
+!CHECK: %[[COORD:.*]] = fir.coordinate_of %[[CB_CONV]], %[[INDEX]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK: %[[CONV:.*]] = fir.convert %[[COORD]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK: %[[CB_MEMBER_1:.*]]:2 = hlfir.declare %[[CONV]] {uniq_name = "_QFmap_mix_of_membersEvar1"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK: %[[CB_CONV:.*]] = fir.convert %[[COMMON_BLOCK]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK: %[[INDEX:.*]] = arith.constant 4 : index
+!CHECK: %[[COORD:.*]] = fir.coordinate_of %[[CB_CONV]], %[[INDEX]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK: %[[CONV:.*]] = fir.convert %[[COORD]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK: %[[CB_MEMBER_2:.*]]:2 = hlfir.declare %[[CONV]] {uniq_name = "_QFmap_mix_of_membersEvar2"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK: %[[MAP_EXP:.*]] = omp.map.info var_ptr(%[[CB_MEMBER_2]]#0 : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "var2"}
+!CHECK: %[[MAP_IMP:.*]] = omp.map.info var_ptr(%[[CB_MEMBER_1]]#1 : !fir.ref<i32>, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref<i32> {name = "var1"}
+!CHECK: omp.target map_entries(%[[MAP_EXP]] -> %[[ARG_EXP:.*]], %[[MAP_IMP]] -> %[[ARG_IMP:.*]] : !fir.ref<i32>, !fir.ref<i32>) {
+!CHECK: ^bb0(%[[ARG_EXP]]: !fir.ref<i32>, %[[ARG_IMP]]: !fir.ref<i32>):
+!CHECK: %[[EXP_MEMBER:.*]]:2 = hlfir.declare %[[ARG_EXP]] {uniq_name = "_QFmap_mix_of_membersEvar2"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK: %[[IMP_MEMBER:.*]]:2 = hlfir.declare %[[ARG_IMP]] {uniq_name = "_QFmap_mix_of_membersEvar1"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+subroutine map_mix_of_members
+ implicit none
+ common /var_common/ var1, var2
+ integer :: var1, var2
+
+!$omp target map(tofrom: var2)
+ var2 = var1
+!$omp end target
+end
+
+!CHECK-LABEL: @_QQmain
+!CHECK: %[[DECL_TAR_CB:.*]] = fir.address_of(@var_common_link_) : !fir.ref<!fir.array<8xi8>>
+!CHECK: %[[MAP_DECL_TAR_CB:.*]] = omp.map.info var_ptr(%[[DECL_TAR_CB]] : !fir.ref<!fir.array<8xi8>>, !fir.array<8xi8>) map_clauses(tofrom) capture(ByRef) -> !fir.ref<!fir.array<8xi8>> {name = "var_common_link"}
+!CHECK: omp.target map_entries(%[[MAP_DECL_TAR_CB]] -> %[[MAP_DECL_TAR_ARG:.*]] : !fir.ref<!fir.array<8xi8>>) {
+!CHECK: ^bb0(%[[MAP_DECL_TAR_ARG]]: !fir.ref<!fir.array<8xi8>>):
+!CHECK: %[[CONV:.*]] = fir.convert %[[MAP_DECL_TAR_ARG]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK: %[[INDEX:.*]] = arith.constant 0 : index
+!CHECK: %[[COORD:.*]] = fir.coordinate_of %[[CONV]], %[[INDEX]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK: %[[CONV:.*]] = fir.convert %[[COORD]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK: %[[MEMBER_ONE:.*]]:2 = hlfir.declare %[[CONV]] {uniq_name = "_QFElink1"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK: %[[CONV:.*]] = fir.convert %[[MAP_DECL_TAR_ARG]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK: %[[INDEX:.*]] = arith.constant 4 : index
+!CHECK: %[[COORD:.*]] = fir.coordinate_of %[[CONV]], %[[INDEX]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK: %[[CONV:.*]] = fir.convert %[[COORD]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK: %[[MEMBER_TWO:.*]]:2 = hlfir.declare %[[CONV]] {uniq_name = "_QFElink2"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+program main
+ implicit none
+ common /var_common_link/ link1, link2
+ integer :: link1, link2
+ !$omp declare target link(/var_common_link/)
+
+!$omp target map(tofrom: /var_common_link/)
+ link1 = link2 + 20
+!$omp end target
+end program
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h b/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
index c9ee0c25194c2..0f078b03457b0 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
+++ b/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
@@ -2110,6 +2110,11 @@ class OpenMPIRBuilder {
int32_t UB);
///}
+ /// Replaces constant values with instruction equivelants where possible
+ /// inside of a function.
+ static void replaceConstantValueUsesInFuncWithInstr(llvm::Value *Input,
+ Function *Func);
+
private:
// Sets the function attributes expected for the outlined function
void setOutlinedTargetRegionFunctionAttributes(Function *OutlinedFn);
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 42ea20919a5e1..4d803e65ae542 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -5085,8 +5085,8 @@ static void replaceConstatExprUsesInFuncWithInstr(ConstantExpr *ConstExpr,
}
}
-static void replaceConstantValueUsesInFuncWithInstr(llvm::Value *Input,
- Function *Func) {
+void OpenMPIRBuilder::replaceConstantValueUsesInFuncWithInstr(
+ llvm::Value *Input, Function *Func) {
for (User *User : make_early_inc_range(Input->users()))
if (auto *Const = dyn_cast<Constant>(User))
if (auto *ConstExpr = dyn_cast<ConstantExpr>(Const))
@@ -5160,6 +5160,31 @@ static Function *createOutlinedFunction(
? make_range(Func->arg_begin() + 1, Func->arg_end())
: Func->args();
+ auto ReplaceValue = [&OMPBuilder](Value *Input, Value *InputCopy,
+ Function *Func) {
+ // Things like GEP's can come in the form of Constants. Constants and
+ // ConstantExpr's do not have access to the knowledge of what they're
+ // contained in, so we must dig a little to find an instruction so we
+ // can tell if they're used inside of the function we're outlining. We
+ // also replace the original constant expression with a new instruction
+ // equivalent; an instruction as it allows easy modification in the
+ // following loop, as we can now know the constant (instruction) is
+ // owned by our target function and replaceUsesOfWith can now be invoked
+ // on it (cannot do this with constants it seems). A brand new one also
+ // allows us to be cautious as it is perhaps possible the old expression
+ // was used inside of the function but exists and is used externally
+ // (unlikely by the nature of a Constant, but still).
+ OMPBuilder.replaceConstantValueUsesInFuncWithInstr(Input, Func);
+
+ // Collect all the instructions
+ for (User *User : make_early_inc_range(Input->users()))
+ if (auto *Instr = dyn_cast<Instruction>(User))
+ if (Instr->getFunction() == Func)
+ Instr->replaceUsesOfWith(Input, InputCopy);
+ };
+
+ SmallVector<std::pair<Value *, Value *>> DeferredReplacement;
+
// Rewrite uses of input valus to parameters.
for (auto InArg : zip(Inputs, ArgRange)) {
Value *Input = std::get<0>(InArg);
@@ -5169,27 +5194,36 @@ static Function *createOutlinedFunction(
Builder.restoreIP(
ArgAccessorFuncCB(Arg, Input, InputCopy, AllocaIP, Builder.saveIP()));
- // Things like GEP's can come in the form of Constants. Constants and
- // ConstantExpr's do not have access to the knowledge of what they're
- // contained in, so we must dig a little to find an instruction so we can
- // tell if they're used inside of the function we're outlining. We also
- // replace the original constant expression with a new instruction
- // equivalent; an instruction as it allows easy modification in the
- // following loop, as we can now know the constant (instruction) is owned by
- // our target function and replaceUsesOfWith can now be invoked on it
- // (cannot do this with constants it seems). A brand new one also allows us
- // to be cautious as it is perhaps possible the old expression was used
- // inside of the function but exists and is used externally (unlikely by the
- // nature of a Constant, but still).
- replaceConstantValueUsesInFuncWithInstr(Input, Func);
+ // In certain cases a Global may be set up for replacement, however, this
+ // Global may be used in multiple arguments to the kernel, just segmented
+ // apart, for example, if we have a global array, that is sectioned into
+ // multiple mappings (technically not legal in OpenMP, but there is a case
+ // in Fortran for Common Blocks where this is neccesary), we will end up
+ // with GEP's into this array inside the kernel, that refer to the Global
+ // but are technically seperate arguments to the kernel for all intents and
+ // purposes. If we have mapped a segment that requires a GEP into the 0-th
+ // index, it will fold into an referal to the Global, if we then encounter
+ // this folded GEP during replacement all of the references to the
+ // Global in the kernel will be replaced with the argument we have generated
+ // that corresponds to it, including any other GEP's that refer to the
+ // Global that may be other arguments. This will invalidate all of the other
+ // preceding mapped arguments that refer to the same global that may be
+ // seperate segments. To prevent this, we defer global processing until all
+ // other processing has been performed.
+ if (llvm::isa<llvm::GlobalValue>(std::get<0>(InArg)) ||
+ llvm::isa<llvm::GlobalObject>(std::get<0>(InArg)) ||
+ llvm::isa<llvm::GlobalVariable>(std::get<0>(InArg))) {
+ DeferredReplacement.push_back(std::make_pair(Input, InputCopy));
+ continue;
+ }
- // Collect all the instructions
- for (User *User : make_early_inc_range(Input->users()))
- if (auto *Instr = dyn_cast<Instruction>(User))
- if (Instr->getFunction() == Func)
- Instr->replaceUsesOfWith(Input, InputCopy);
+ ReplaceValue(Input, InputCopy, Func);
}
+ // Replace all of our deferred Input values, currently just Globals.
+ for (auto Deferred : DeferredReplacement)
+ ReplaceValue(std::get<0>(Deferred), std::get<1>(Deferred), Func);
+
// Restore insert point.
Builder.restoreIP(OldInsertPoint);
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 0c34126667324..d2b4d77cfdcb8 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -2021,12 +2021,6 @@ llvm::Value *getSizeInBytes(DataLayout &dl, const mlir::Type &type,
Operation *clauseOp, llvm::Value *basePointer,
llvm::Type *baseType, llvm::IRBuilderBase &builder,
LLVM::ModuleTranslation &moduleTranslation) {
- // utilising getTypeSizeInBits instead of getTypeSize as getTypeSize gives
- // the size in inconsistent byte or bit format.
- uint64_t underlyingTypeSzInBits = dl.getTypeSizeInBits(type);
- if (auto arrTy = llvm::dyn_cast_if_present<LLVM::LLVMArrayType>(type))
- underlyingTypeSzInBits = getArrayElementSizeInBits(arrTy, dl);
-
if (auto memberClause =
mlir::dyn_cast_if_present<mlir::omp::MapInfoOp>(clauseOp)) {
// This calculates the size to transfer based on bounds and the underlying
@@ -2052,6 +2046,12 @@ llvm::Value *getSizeInBytes(DataLayout &dl, const mlir::Type &type,
}
}
+ // utilising getTy...
[truncated]
|
|
@kiranchandramohan No real need to review this, but I've added you as a reviewer incase you wish to feed into it, but also if you know someone else that might be quite helpful to review the change set external to AMD! In particular for this change: https://github.com/llvm/llvm-project/pull/91829/files#diff-496a295679ae3c43f8651c944a1bd9dca177ad2b5e4d7121f96938024e292bc1R841 it seems reasonable to me to do, but someone with more knowledge on common blocks and their implementation may know otherwise. |
|
Small ping for some reviews on this in the near future! |
|
Small ping on this for some reviewer attention if at all possible! |
|
I will have a look this week. Sorry for the delay here. |
|
Was there an initial patch that added support for common block mapping? If so, could you give me a link? |
There was not, I believe we just assumed it was working after some prior testing but the tests were not quite correct unfortunately, so this PR allows it to appropriately function when declare target link'd (but no declare target to) and regular mapped explicit/implicit at least from testing so far. |
agozillon
force-pushed
the
common-block-map-pr
branch
from
0c09c3a
to
2fe98b9
Compare
|
Rebased and fixed conflicts on the PR and updated this PR on top of the replaceConstant PR which upgrades the variation I had in this PR. Basically just simplifies this PR by 30-40 lines. However, I did encounter a bit of an issue that seems unrelated to this PR that's arisen between now and the original opening of this PR, it seems that utilising the common block /var/ syntax inside of the map clause will crash the compiler in the decomposer, it's not picked up by any of the upstream tests at the moment as the main map clause + common block tests are semantic checks at the moment, and the act of emitting semantic erroring in the one test I seen prevents the test from progressing to the point it would crash! I've talked to @kparzysz and will open up a very small PR to fix it in a little bit. However, in the meantime, please do feel free to review this PR and if you see errors in the CI in the meantime, that's likely the cause. |
|
Small ping for some reviewer attention, would be nice to be able to progress this PR along towards some kind of resolution! Thank you very much ahead of time :-) |
The link you have given here refers to existing code. Is that correct? If you can phrase your exact question on the issue with the common block then we can request Jean or someone else to have a look. |
Ah, the link location seems to have changed after the rebase and updates, sorry about that. This is the location now (the definition of I am wondering if it is reasonable for me to generate common block member accesses within the target region and then bind the symbol for the common block members to them for the duration of the target region lowering. This or something very similar is currently done for another piece of OpenMP code related to privatization, so it's borrowing a similar concept. It results in the appropriate member accesses to the mapped common block as well as the correct usage of the newly generated local member accesses throughout the target operation. Without this step you end up with member accesses external to the IFA target region which results in verification issues, an alternative more complex and less fool proof (for me at least) method would be to clone the existing member accesses into the region and rebind them to the corresponding blocks argument for the mapped common block. This is an issue when using the common block symbol (e.g. But in essence, I thought this method might be reasonable as it's been utilised before (albeit in a slightly different scenario) and it was a fair bit simpler than the alternatives. I'd just like to verify if it seemed reasonable to those more familiar with FIR and Fortran :-)! The above method results in the following IR when mapping a common block symbol (not individual or multiple members, that is the intent at least): https://github.com/llvm/llvm-project/pull/91829/files#diff-291898b3de7308afe65947c1029376201d17c7598f96adc09d43548cd9364548R9 |
For common blocks the OpenMP 5.2 standard in Section 3.2.1 Page 61 and Section 5.4 Page 108 has the following. I think privatization uses this description to privatize each member and hence have separate binding. In your proposal, when mapping the commonblock I am assuming each member get mapped separately. This is alrite as per the first point above. There could be a performance issue. Also assuming lowering to LLVM IR ensure that while each member is mapped separately, it is actually mapped offsets of the original commonblock. |
Yes, I seen the first exert, but not the latter, so wasn't so sure of the details behind that choice! In the case of this PR where we are mapping a common block (using the common block symbol) into a target region, we're effectively mapping the entire block in one map, and we are generating accesses into this mapped common block to retrieve the individual members which are then bound to the original member symbols for the duration of the target operation lowering so that whenever those symbols are referenced we effectively access the members from the mapped common block member, that's my understanding of it at least! So I don't believe we are doing quite the same thing (no more than map already does at least with it being considered a data transfer to and from storage on a device) it's primarily as a way to have a seperate access to the members within the target region rather than external to it that directly accesses the mapped common block storage!
However, in the case where you perform a mapping using a common block symbol e.g. map(tofrom: /commonblock/) the mapping will be as if you map the entire block of memory, not as if you map each individually in this PR. You can see this in the tests e.g.: https://github.com/llvm/llvm-project/pull/91829/files#diff-0b2da1d2f88526ca7701608cd4b7478ea4a4c067abe549e90f04b798947498c4R236
This occurs in this PR when you map individual members (but the block is mapped as a whole in the case of a common block map symbol): https://github.com/llvm/llvm-project/pull/91829/files#diff-0b2da1d2f88526ca7701608cd4b7478ea4a4c067abe549e90f04b798947498c4R383 The above choices were simpler from the perspective of how the IR in the target region is currently generated when the common block symbol is provided as well as with the eventual lowering to LLVM-IR with the way common blocks are implemented as a block of bytes that is indexed into per member. I am happy to try and implement it as individual mappings however in both cases, if that's the direction we wish to go! But the problem then becomes how to maintain or re-find the common block symbol from the individual members being mapped when we perform the LLVM-IR lowering later, perhaps I could try to treat it as a form of derived type/structure member mapping, but that might end up with more unnecessary complexity than is required for the mapping of common blocks, However, I am unfortunately not sure of the possible consequences of mapping it as a single block of memory as opposed to individual mappings, maybe @mjklemm, yourself or another more OpenMP+Fortran specification savvy person would be able to point them out! |
|
I think this makes perfect sense to me. Thanks for the explanation. |
|
Thank you very much @kiranchandramohan @raghavendhra I'll aim to land the PR on Monday or Tuesday next week incase anyone has anything else they would like to add. I'll likely rebase the PR and add one or two more tests before I land it. |
agozillon
force-pushed
the
common-block-map-pr
branch
from
2fe98b9
to
b98c4a3
Compare
|
Added a couple more tests, primarily extending convert-to-llvm-openmp-and-fir.fir and adding a new test to mlir/test/Target/LLVMIR named omptarget-fortran-common-block-host.mlir. The convert-to-llvm-openmp-and-fir.fir test is dependent on the changes in this PR: #96530 which just adds the cg-rewrite pass to the fir-opt command for the convert-to-llvm-openmp-and-fir.fir test, allowing DeclareOp's to be utilised in the test without verification errors. So I'll hold off landing the PR until that has hopefully landed. Otherwise, if no further comments over the new test additions are made or any other comments in the meantime I will go ahead with landing the PR. |
…e target link This PR attempts to fix common block mapping for regular mapping of these types as well as when they have been marked as "declare target link". This PR should allow correct mapping of both the members of a common block and the full common block via its block symbol. The main changes were some adjustments to the Fortran OpenMP lowering to HLFIR/FIR, the lowering of the LLVM+OpenMP dialect to LLVM-IR and adjustments to the way the we handle target kernel map argument rebinding inside of the OMPIRBuilder. For the Fortran OpenMP lowering were two changes, one to prevent the implicit capture of common block members when the common block symbol itself has been marked and the other creates intermediate member access inside of the target region to be used in-place of those external to the target region, this prevents external usages breaking the IsolatedFromAbove pact. In the latter case, there was an adjustment to the size calculation for types to better handle cases where we pass an array as the type of a map (as opposed to the bounds and the type of the element), which occurs in the case of common blocks. There is also some adjustment to how handleDeclareTargetMapVar handles renaming of declare target symbols in the module to the reference pointer, now it will only apply to those within the kernel that is currently being generated and we also perform a modification to replace constants with instructions as necessary as we cannot replace these with our reference pointer (non-constant and constants do not mix nicely). In the case of the OpenMPIRBuilder some changes were mde to defer global symbol rebinding to kernel arguments until all other arguments have been rebound. This makes sure we do not replace uses that may refer to the global (e.g. a GEP) but are themselves actually a separate argument that needs bound. Currently "declare target to" still needs some work, but this may be the case for all types in conjunction with "declare target to" at the moment.
…uction transformation code with newer variation from recently landed PR
…to offload folder
|
Going to adjust the offending test to not need the cg-rewrite pass change and then land the PR in the next hour after it passes the linux buildbot (and perhaps windows if it doesn't take an excessive amount of time). So, please do add any further comments in the next little bit incase you wish a delay/fix of the PR or a revert. |
agozillon
force-pushed
the
common-block-map-pr
branch
from
39ddaff
to
892b725
Compare
…e target link (llvm#91829) This PR attempts to fix common block mapping for regular mapping of these types as well as when they have been marked as "declare target link". This PR should allow correct mapping of both the members of a common block and the full common block via its block symbol. The main changes were some adjustments to the Fortran OpenMP lowering to HLFIR/FIR, the lowering of the LLVM+OpenMP dialect to LLVM-IR and adjustments to the way the we handle target kernel map argument rebinding inside of the OMPIRBuilder. For the Fortran OpenMP lowering were two changes, one to prevent the implicit capture of common block members when the common block symbol itself has been marked and the other creates intermediate member access inside of the target region to be used in-place of those external to the target region, this prevents external usages breaking the IsolatedFromAbove pact. In the latter case, there was an adjustment to the size calculation for types to better handle cases where we pass an array as the type of a map (as opposed to the bounds and the type of the element), which occurs in the case of common blocks. There is also some adjustment to how handleDeclareTargetMapVar handles renaming of declare target symbols in the module to the reference pointer, now it will only apply to those within the kernel that is currently being generated and we also perform a modification to replace constants with instructions as necessary as we cannot replace these with our reference pointer (non-constant and constants do not mix nicely). In the case of the OpenMPIRBuilder some changes were made to defer global symbol rebinding to kernel arguments until all other arguments have been rebound. This makes sure we do not replace uses that may refer to the global (e.g. a GEP) but are themselves actually a separate argument that needs bound. Currently "declare target to" still needs some work, but this may be the case for all types in conjunction with "declare target to" at the moment.
This PR attempts to fix common block mapping for regular mapping of these types as well as when they have been marked as "declare target link". This PR should allow correct mapping of both the members of a common block and the full common block via its block symbol.
The main changes were some adjustments to the Fortran OpenMP lowering to HLFIR/FIR, the lowering of the LLVM+OpenMP dialect to LLVM-IR and adjustments to the way the we handle target kernel map argument rebinding inside of the OMPIRBuilder.
For the Fortran OpenMP lowering were two changes, one to prevent the implicit capture of common block members when the common block symbol itself has been marked and the other creates intermediate member access inside of the target region to be used in-place of those external to the target region, this prevents external usages breaking the IsolatedFromAbove pact.
In the latter case, there was an adjustment to the size calculation for types to better handle cases where we pass an array as the type of a map (as opposed to the bounds and the type of the element), which occurs in the case of common blocks. There is also some adjustment to how handleDeclareTargetMapVar handles renaming of declare target symbols in the module to the reference pointer, now it will only apply to those within the kernel that is currently being generated and we also perform a modification to replace constants with instructions as necessary as we cannot replace these with our reference pointer (non-constant and constants do not mix nicely).
In the case of the OpenMPIRBuilder some changes were made to defer global symbol rebinding to kernel arguments until all other arguments have been rebound. This makes sure we do not replace uses that may refer to the global (e.g. a GEP) but are themselves actually a separate argument that needs bound.
Currently "declare target to" still needs some work, but this may be the case for all types in conjunction with "declare target to" at the moment.