[OpenMP]Update use_device_clause lowering #101707
Merged
Conversation
This file contains 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
TIFitis
requested review from
skatrak,
kiranchandramohan,
raghavendhra,
agozillon and
kiranktp
llvmbot
added
mlir:llvm
mlir
mlir:openmp
flang:openmp
clang:openmp
|
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-flang-openmp Author: Akash Banerjee (TIFitis) ChangesThis patch updates the use_device_ptr and use_device_addr clauses to use the mapInfoOps for lowering. This allows all the types that are handle by the map clauses such as derived types to also be supported by the use_device_clauses. This is patch 2/2 in a series of patches. Patch is 23.95 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/101707.diff 4 Files Affected:
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 77e350e7276ab..5350daa34b6bb 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -6351,7 +6351,7 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createTargetData(
// Disable TargetData CodeGen on Device pass.
if (Config.IsTargetDevice.value_or(false)) {
if (BodyGenCB)
- Builder.restoreIP(BodyGenCB(Builder.saveIP(), BodyGenTy::NoPriv));
+ Builder.restoreIP(BodyGenCB(CodeGenIP, BodyGenTy::NoPriv));
return Builder.saveIP();
}
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index ddee117838697..dd2ecbb1f3532 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -2110,6 +2110,8 @@ getRefPtrIfDeclareTarget(mlir::Value value,
struct MapInfoData : llvm::OpenMPIRBuilder::MapInfosTy {
llvm::SmallVector<bool, 4> IsDeclareTarget;
llvm::SmallVector<bool, 4> IsAMember;
+ // Identify if mapping was added by mapClause or use_device clauses.
+ llvm::SmallVector<bool, 4> IsAMapping;
llvm::SmallVector<mlir::Operation *, 4> MapClause;
llvm::SmallVector<llvm::Value *, 4> OriginalValue;
// Stripped off array/pointer to get the underlying
@@ -2193,62 +2195,125 @@ llvm::Value *getSizeInBytes(DataLayout &dl, const mlir::Type &type,
return builder.getInt64(dl.getTypeSizeInBits(type) / 8);
}
-void collectMapDataFromMapVars(MapInfoData &mapData,
- llvm::SmallVectorImpl<Value> &mapVars,
- LLVM::ModuleTranslation &moduleTranslation,
- DataLayout &dl, llvm::IRBuilderBase &builder) {
+void collectMapDataFromMapOperands(
+ MapInfoData &mapData, llvm::SmallVectorImpl<Value> &mapVars,
+ LLVM::ModuleTranslation &moduleTranslation, DataLayout &dl,
+ llvm::IRBuilderBase &builder,
+ const llvm::ArrayRef<Value> &useDevPtrOperands = {},
+ const llvm::ArrayRef<Value> &useDevAddrOperands = {}) {
+ // Process MapOperands
for (mlir::Value mapValue : mapVars) {
- if (auto mapOp = mlir::dyn_cast_if_present<mlir::omp::MapInfoOp>(
- mapValue.getDefiningOp())) {
- mlir::Value offloadPtr =
- mapOp.getVarPtrPtr() ? mapOp.getVarPtrPtr() : mapOp.getVarPtr();
- mapData.OriginalValue.push_back(
- moduleTranslation.lookupValue(offloadPtr));
- mapData.Pointers.push_back(mapData.OriginalValue.back());
-
- if (llvm::Value *refPtr =
- getRefPtrIfDeclareTarget(offloadPtr,
- moduleTranslation)) { // declare target
- mapData.IsDeclareTarget.push_back(true);
- mapData.BasePointers.push_back(refPtr);
- } else { // regular mapped variable
- mapData.IsDeclareTarget.push_back(false);
- mapData.BasePointers.push_back(mapData.OriginalValue.back());
- }
+ auto mapOp = mlir::cast<mlir::omp::MapInfoOp>(mapValue.getDefiningOp());
+ mlir::Value offloadPtr =
+ mapOp.getVarPtrPtr() ? mapOp.getVarPtrPtr() : mapOp.getVarPtr();
+ mapData.OriginalValue.push_back(moduleTranslation.lookupValue(offloadPtr));
+ mapData.Pointers.push_back(mapData.OriginalValue.back());
+
+ if (llvm::Value *refPtr =
+ getRefPtrIfDeclareTarget(offloadPtr,
+ moduleTranslation)) { // declare target
+ mapData.IsDeclareTarget.push_back(true);
+ mapData.BasePointers.push_back(refPtr);
+ } else { // regular mapped variable
+ mapData.IsDeclareTarget.push_back(false);
+ mapData.BasePointers.push_back(mapData.OriginalValue.back());
+ }
- mapData.BaseType.push_back(
- moduleTranslation.convertType(mapOp.getVarType()));
- mapData.Sizes.push_back(
- getSizeInBytes(dl, mapOp.getVarType(), mapOp, mapData.Pointers.back(),
- mapData.BaseType.back(), builder, moduleTranslation));
- mapData.MapClause.push_back(mapOp.getOperation());
- mapData.Types.push_back(
- llvm::omp::OpenMPOffloadMappingFlags(mapOp.getMapType().value()));
- mapData.Names.push_back(LLVM::createMappingInformation(
- mapOp.getLoc(), *moduleTranslation.getOpenMPBuilder()));
- mapData.DevicePointers.push_back(
- llvm::OpenMPIRBuilder::DeviceInfoTy::None);
-
- // Check if this is a member mapping and correctly assign that it is, if
- // it is a member of a larger object.
- // TODO: Need better handling of members, and distinguishing of members
- // that are implicitly allocated on device vs explicitly passed in as
- // arguments.
- // TODO: May require some further additions to support nested record
- // types, i.e. member maps that can have member maps.
- mapData.IsAMember.push_back(false);
- for (mlir::Value mapValue : mapVars) {
- if (auto map = mlir::dyn_cast_if_present<mlir::omp::MapInfoOp>(
- mapValue.getDefiningOp())) {
- for (auto member : map.getMembers()) {
- if (member == mapOp) {
- mapData.IsAMember.back() = true;
- }
+ mapData.BaseType.push_back(
+ moduleTranslation.convertType(mapOp.getVarType()));
+ mapData.Sizes.push_back(
+ getSizeInBytes(dl, mapOp.getVarType(), mapOp, mapData.Pointers.back(),
+ mapData.BaseType.back(), builder, moduleTranslation));
+ mapData.MapClause.push_back(mapOp.getOperation());
+ mapData.Types.push_back(
+ llvm::omp::OpenMPOffloadMappingFlags(mapOp.getMapType().value()));
+ mapData.Names.push_back(LLVM::createMappingInformation(
+ mapOp.getLoc(), *moduleTranslation.getOpenMPBuilder()));
+ mapData.DevicePointers.push_back(llvm::OpenMPIRBuilder::DeviceInfoTy::None);
+ mapData.IsAMapping.push_back(true);
+
+ // Check if this is a member mapping and correctly assign that it is, if
+ // it is a member of a larger object.
+ // TODO: Need better handling of members, and distinguishing of members
+ // that are implicitly allocated on device vs explicitly passed in as
+ // arguments.
+ // TODO: May require some further additions to support nested record
+ // types, i.e. member maps that can have member maps.
+ mapData.IsAMember.push_back(false);
+ for (mlir::Value mapValue : mapVars) {
+ if (auto map = mlir::dyn_cast_if_present<mlir::omp::MapInfoOp>(
+ mapValue.getDefiningOp())) {
+ for (auto member : map.getMembers()) {
+ if (member == mapOp) {
+ mapData.IsAMember.back() = true;
}
}
}
}
}
+
+ auto findMapInfo = [&mapData](llvm::Value *val,
+ llvm::OpenMPIRBuilder::DeviceInfoTy devInfoTy) {
+ unsigned index = 0;
+ bool found = false;
+ for (llvm::Value *basePtr : mapData.OriginalValue) {
+ if (basePtr == val && mapData.IsAMapping[index]) {
+ found = true;
+ mapData.Types[index] |=
+ llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_RETURN_PARAM;
+ mapData.DevicePointers[index] = devInfoTy;
+ }
+ index++;
+ }
+ return found;
+ };
+
+ // Process useDevPtr(Addr)Operands
+ auto addDevInfos = [&](const llvm::ArrayRef<Value> &useDevOperands,
+ llvm::OpenMPIRBuilder::DeviceInfoTy devInfoTy) {
+ for (mlir::Value mapValue : useDevOperands) {
+ auto mapOp = mlir::cast<mlir::omp::MapInfoOp>(mapValue.getDefiningOp());
+ mlir::Value offloadPtr =
+ mapOp.getVarPtrPtr() ? mapOp.getVarPtrPtr() : mapOp.getVarPtr();
+ llvm::Value *origValue = moduleTranslation.lookupValue(offloadPtr);
+
+ // Check if map info is already present for this entry.
+ if (!findMapInfo(origValue, devInfoTy)) {
+ mapData.OriginalValue.push_back(origValue);
+ mapData.Pointers.push_back(mapData.OriginalValue.back());
+ mapData.IsDeclareTarget.push_back(false);
+ mapData.BasePointers.push_back(mapData.OriginalValue.back());
+ mapData.BaseType.push_back(
+ moduleTranslation.convertType(mapOp.getVarType()));
+ mapData.Sizes.push_back(builder.getInt64(0));
+ mapData.MapClause.push_back(mapOp.getOperation());
+ mapData.Types.push_back(
+ llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_RETURN_PARAM);
+ mapData.Names.push_back(LLVM::createMappingInformation(
+ mapOp.getLoc(), *moduleTranslation.getOpenMPBuilder()));
+ mapData.DevicePointers.push_back(devInfoTy);
+ mapData.IsAMapping.push_back(true);
+
+ // Check if this is a member mapping and correctly assign that it is,
+ // if it is a member of a larger object.
+ // TODO: Need better handling of members, and distinguishing of
+ // members that are implicitly allocated on device vs explicitly
+ // passed in as arguments.
+ // TODO: May require some further additions to support nested record
+ // types, i.e. member maps that can have member maps.
+ mapData.IsAMember.push_back(false);
+ for (mlir::Value mapValue : useDevOperands)
+ if (auto map = mlir::dyn_cast_if_present<mlir::omp::MapInfoOp>(
+ mapValue.getDefiningOp()))
+ for (auto member : map.getMembers())
+ if (member == mapOp)
+ mapData.IsAMember.back() = true;
+ }
+ }
+ };
+
+ addDevInfos(useDevPtrOperands, llvm::OpenMPIRBuilder::DeviceInfoTy::Pointer);
+ addDevInfos(useDevAddrOperands, llvm::OpenMPIRBuilder::DeviceInfoTy::Address);
}
static int getMapDataMemberIdx(MapInfoData &mapData,
@@ -2426,7 +2491,7 @@ static llvm::omp::OpenMPOffloadMappingFlags mapParentWithMembers(
? llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TARGET_PARAM
: llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE);
combinedInfo.DevicePointers.emplace_back(
- llvm::OpenMPIRBuilder::DeviceInfoTy::None);
+ mapData.DevicePointers[mapDataIndex]);
combinedInfo.Names.emplace_back(LLVM::createMappingInformation(
mapData.MapClause[mapDataIndex]->getLoc(), ompBuilder));
combinedInfo.BasePointers.emplace_back(mapData.BasePointers[mapDataIndex]);
@@ -2553,7 +2618,7 @@ static void processMapMembersWithParent(
combinedInfo.Types.emplace_back(mapFlag);
combinedInfo.DevicePointers.emplace_back(
- llvm::OpenMPIRBuilder::DeviceInfoTy::None);
+ mapData.DevicePointers[memberDataIdx]);
combinedInfo.Names.emplace_back(
LLVM::createMappingInformation(memberClause.getLoc(), ompBuilder));
combinedInfo.BasePointers.emplace_back(mapData.BasePointers[mapDataIndex]);
@@ -2714,10 +2779,7 @@ static void genMapInfos(llvm::IRBuilderBase &builder,
LLVM::ModuleTranslation &moduleTranslation,
DataLayout &dl,
llvm::OpenMPIRBuilder::MapInfosTy &combinedInfo,
- MapInfoData &mapData,
- const SmallVector<Value> &useDevicePtrVars = {},
- const SmallVector<Value> &useDeviceAddrVars = {},
- bool isTargetParams = false) {
+ MapInfoData &mapData, bool isTargetParams = false) {
// We wish to modify some of the methods in which arguments are
// passed based on their capture type by the target region, this can
// involve generating new loads and stores, which changes the
@@ -2734,15 +2796,6 @@ static void genMapInfos(llvm::IRBuilderBase &builder,
llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
- auto fail = [&combinedInfo]() -> void {
- combinedInfo.BasePointers.clear();
- combinedInfo.Pointers.clear();
- combinedInfo.DevicePointers.clear();
- combinedInfo.Sizes.clear();
- combinedInfo.Types.clear();
- combinedInfo.Names.clear();
- };
-
// We operate under the assumption that all vectors that are
// required in MapInfoData are of equal lengths (either filled with
// default constructed data or appropiate information) so we can
@@ -2763,46 +2816,6 @@ static void genMapInfos(llvm::IRBuilderBase &builder,
processIndividualMap(mapData, i, combinedInfo, isTargetParams);
}
-
- auto findMapInfo = [&combinedInfo](llvm::Value *val, unsigned &index) {
- index = 0;
- for (llvm::Value *basePtr : combinedInfo.BasePointers) {
- if (basePtr == val)
- return true;
- index++;
- }
- return false;
- };
-
- auto addDevInfos = [&, fail](auto useDeviceVars, auto devOpType) -> void {
- for (const auto &useDeviceVar : useDeviceVars) {
- // TODO: Only LLVMPointerTypes are handled.
- if (!isa<LLVM::LLVMPointerType>(useDeviceVar.getType()))
- return fail();
-
- llvm::Value *mapOpValue = moduleTranslation.lookupValue(useDeviceVar);
-
- // Check if map info is already present for this entry.
- unsigned infoIndex;
- if (findMapInfo(mapOpValue, infoIndex)) {
- combinedInfo.Types[infoIndex] |=
- llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_RETURN_PARAM;
- combinedInfo.DevicePointers[infoIndex] = devOpType;
- } else {
- combinedInfo.BasePointers.emplace_back(mapOpValue);
- combinedInfo.Pointers.emplace_back(mapOpValue);
- combinedInfo.DevicePointers.emplace_back(devOpType);
- combinedInfo.Names.emplace_back(
- LLVM::createMappingInformation(useDeviceVar.getLoc(), *ompBuilder));
- combinedInfo.Types.emplace_back(
- llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_RETURN_PARAM);
- combinedInfo.Sizes.emplace_back(builder.getInt64(0));
- }
- }
- };
-
- addDevInfos(useDevicePtrVars, llvm::OpenMPIRBuilder::DeviceInfoTy::Pointer);
- addDevInfos(useDeviceAddrVars, llvm::OpenMPIRBuilder::DeviceInfoTy::Address);
}
static LogicalResult
@@ -2899,19 +2912,15 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
using InsertPointTy = llvm::OpenMPIRBuilder::InsertPointTy;
MapInfoData mapData;
- collectMapDataFromMapVars(mapData, mapVars, moduleTranslation, DL, builder);
+ collectMapDataFromMapOperands(mapData, mapVars, moduleTranslation, DL,
+ builder, useDevicePtrVars, useDeviceAddrVars);
// Fill up the arrays with all the mapped variables.
llvm::OpenMPIRBuilder::MapInfosTy combinedInfo;
auto genMapInfoCB =
[&](InsertPointTy codeGenIP) -> llvm::OpenMPIRBuilder::MapInfosTy & {
builder.restoreIP(codeGenIP);
- if (auto dataOp = dyn_cast<omp::TargetDataOp>(op)) {
- genMapInfos(builder, moduleTranslation, DL, combinedInfo, mapData,
- useDevicePtrVars, useDeviceAddrVars);
- } else {
- genMapInfos(builder, moduleTranslation, DL, combinedInfo, mapData);
- }
+ genMapInfos(builder, moduleTranslation, DL, combinedInfo, mapData);
return combinedInfo;
};
@@ -2930,21 +2939,23 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
if (!info.DevicePtrInfoMap.empty()) {
builder.restoreIP(codeGenIP);
unsigned argIndex = 0;
- for (auto &devPtrOp : useDevicePtrVars) {
- llvm::Value *mapOpValue = moduleTranslation.lookupValue(devPtrOp);
- const auto &arg = region.front().getArgument(argIndex);
- moduleTranslation.mapValue(arg,
- info.DevicePtrInfoMap[mapOpValue].second);
- argIndex++;
- }
-
- for (auto &devAddrOp : useDeviceAddrVars) {
- llvm::Value *mapOpValue = moduleTranslation.lookupValue(devAddrOp);
- const auto &arg = region.front().getArgument(argIndex);
- auto *LI = builder.CreateLoad(
- builder.getPtrTy(), info.DevicePtrInfoMap[mapOpValue].second);
- moduleTranslation.mapValue(arg, LI);
- argIndex++;
+ for (size_t i = 0; i < combinedInfo.BasePointers.size(); ++i) {
+ if (combinedInfo.DevicePointers[i] ==
+ llvm::OpenMPIRBuilder::DeviceInfoTy::Pointer) {
+ const auto &arg = region.front().getArgument(argIndex);
+ moduleTranslation.mapValue(
+ arg,
+ info.DevicePtrInfoMap[combinedInfo.BasePointers[i]].second);
+ argIndex++;
+ } else if (combinedInfo.DevicePointers[i] ==
+ llvm::OpenMPIRBuilder::DeviceInfoTy::Address) {
+ const auto &arg = region.front().getArgument(argIndex);
+ auto *loadInst = builder.CreateLoad(
+ builder.getPtrTy(),
+ info.DevicePtrInfoMap[combinedInfo.BasePointers[i]].second);
+ moduleTranslation.mapValue(arg, loadInst);
+ argIndex++;
+ }
}
bodyGenStatus = inlineConvertOmpRegions(region, "omp.data.region",
@@ -2957,6 +2968,21 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
// If device info is available then region has already been generated
if (info.DevicePtrInfoMap.empty()) {
builder.restoreIP(codeGenIP);
+ // For device pass, if use_device_ptr(addr) mappings were present,
+ // we need to link them here before codegen.
+ if (ompBuilder->Config.IsTargetDevice.value_or(false)) {
+ unsigned argIndex = 0;
+ for (size_t i = 0; i < mapData.BasePointers.size(); ++i) {
+ if (mapData.DevicePointers[i] ==
+ llvm::OpenMPIRBuilder::DeviceInfoTy::Pointer ||
+ mapData.DevicePointers[i] ==
+ llvm::OpenMPIRBuilder::DeviceInfoTy::Address) {
+ const auto &arg = region.front().getArgument(argIndex);
+ moduleTranslation.mapValue(arg, mapData.BasePointers[i]);
+ argIndex++;
+ }
+ }
+ }
bodyGenStatus = inlineConvertOmpRegions(region, "omp.data.region",
builder, moduleTranslation);
}
@@ -3292,14 +3318,14 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
findAllocaInsertPoint(builder, moduleTranslation);
MapInfoData mapData;
- collectMapDataFromMapVars(mapData, mapVars, moduleTranslation, dl, builder);
+ collectMapDataFromMapOperands(mapData, mapVars, moduleTranslation, dl,
+ builder);
llvm::OpenMPIRBuilder::MapInfosTy combinedInfos;
auto genMapInfoCB = [&](llvm::OpenMPIRBuilder::InsertPointTy codeGenIP)
-> llvm::OpenMPIRBuilder::MapInfosTy & {
builder.restoreIP(codeGenIP);
- genMapInfos(builder, moduleTranslation, dl, combinedInfos, mapData, {}, {},
- true);
+ genMapInfos(builder, moduleTranslation, dl, combinedInfos, mapData, true);
return combinedInfos;
};
diff --git a/mlir/test/Target/LLVMIR/omptarget-llvm.mlir b/mlir/test/Target/LLVMIR/omptarget-llvm.mlir
index bf9fa183bfb80..458d2f28a78f8 100644
--- a/mlir/test/Target/LLVMIR/omptarget-llvm.mlir
+++ b/mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -209,7 +209,8 @@ llvm.func @_QPopenmp_target_use_dev_ptr() {
%0 = llvm.mlir.constant(1 : i64) : i64
%a = llvm.alloca %0 x !llvm.ptr : (i64) -> !llvm.ptr
%map1 = omp.map.info var_ptr(%a : !llvm.ptr, !llvm.ptr) map_clauses(from) capture(ByRef) -> !llvm.ptr {name = ""}
- omp.target_data map_entries(%map1 : !llvm.ptr) use_device_ptr(%a : !llvm.ptr) {
+ %map2 = omp.map.info var_ptr(%a : !llvm.ptr, !llvm.ptr) map_clauses(from) capture(ByRef) -> !llvm.ptr {name = ""}
+ omp.target_data map_entries(%map1 : !llvm.ptr) use_device_ptr(%map2 : !llvm.ptr) {
^bb0(%arg0: !llvm.ptr):
%1 = llvm.mlir.constant(10 : i32) : i32
%2 = llvm.load %arg0 : !llvm.ptr -> !llvm.ptr
@@ -253,7 +254,8 @@ llvm.func @_QPopenmp_target_use_dev_addr() {
%0 = llvm.mlir.constant(1 : i64) : i64
%a = llvm.alloca %0 x !llvm.ptr : (i64) -> !llvm.ptr
%map = omp.map.info var_ptr(%a : !llvm.ptr, !llvm.ptr) map_clauses(from) capture(ByRef) -> !llvm.ptr {name = ""}
- omp.target_data map_entries(%map : !llvm.ptr) use_device_addr(%a : !llvm.ptr) {
+ %map2 = omp.map.info var_ptr(%a : !llvm.ptr, !llvm.ptr) map_clauses(from) capture(ByRef) -> !llvm.ptr {name = ""}
+ omp.target_data map_entries(%map : !llvm.ptr)...
[truncated]
|
TIFitis
force-pushed
the
users/akash/use-device-lowering-1
branch
from
d996277 to
74535f5
Compare
TIFitis
force-pushed
the
users/akash/use-device-lowering-2
branch
from
945337b to
9db1951
Compare
TIFitis
force-pushed
the
users/akash/use-device-lowering-1
branch
from
74535f5 to
7e4b6d7
Compare
TIFitis
force-pushed
the
users/akash/use-device-lowering-2
branch
from
9db1951 to
3a2afe7
Compare
Dinistro
reviewed
Aug 20, 2024
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Outdated
Show resolved
Hide resolved
|
Thanks @Dinistro for the comments, I've addressed them in the latest revision. |
|
As I'm not too familiar with these parts of MLIR, I'll not be able to review this properly. Does someone else have capacity to do a pass on the logic of this? |
skatrak
reviewed
Aug 21, 2024
| @@ -2439,7 +2504,7 @@ static llvm::omp::OpenMPOffloadMappingFlags mapParentWithMembers( | |||
| // data by the descriptor (which itself, is a structure containing | |||
| // runtime information on the dynamically allocated data). | |||
| auto parentClause = | |||
| llvm::cast<mlir::omp::MapInfoOp>(mapData.MapClause[mapDataIndex]); | |||
| llvm::cast<omp::MapInfoOp>(mapData.MapClause[mapDataIndex]); | |||
There was a problem hiding this comment.
Nit: Perhaps not for this PR, but it seems like llvm::SmallVector<mlir::omp::MapInfoOp> would be a more appropriate type for MapInfoData::MapClause, and avoid doing all these casts everywhere it's used.
There was a problem hiding this comment.
Agreed, I'll create an NFC PR to address this.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Outdated
Show resolved
Hide resolved
Comment on lines
6
to
7
| // CHECK-NOT: call void @__tgt_target_data_begin_mapper | ||
| // CHECK: store i32 999, ptr {{.*}} |
There was a problem hiding this comment.
I think the test can be extended a bit.
TIFitis
force-pushed
the
users/akash/use-device-lowering-2
branch
from
c62fe42 to
9cb5045
Compare
|
@skatrak Thanks for the review, I've addressed the comments in the latest revision. |
|
Ping for review. |
skatrak
approved these changes
Aug 29, 2024
| // types, i.e. member maps that can have member maps. | ||
| mapData.IsAMember.push_back(false); | ||
| for (Value mapValue : mapVars) { | ||
| auto map = cast<omp::MapInfoOp>(mapValue.getDefiningOp()); |
There was a problem hiding this comment.
Nit: Perhaps rename to otherMap or something like that to help readability? Also, should we check that the inner mapValue is not the same as the outer one before iterating over the members of the defining omp.map.info?
There was a problem hiding this comment.
I've moved the logic to a lambda, hopefully that helps readability along with code divergence.
I don't think we need to check for member mapOp being same as parent as that would be a peculiar scenario we aren't expecting.
This patch updates the use_device_ptr and use_device_addr clauses to use the mapInfoOps for lowering. This allows all the types that are handle by the map clauses such as derived types to also be supported by the use_device_clauses. This is patch 1/2 in a series of patches. Co-authored-by: Raghu Maddhipatla raghu.maddhipatla@amd.com
TIFitis
force-pushed
the
users/akash/use-device-lowering-1
branch
from
f6a6b00 to
b8d523a
Compare
This patch updates the use_device_ptr and use_device_addr clauses to use the mapInfoOps for lowering. This allows all the types that are handle by the map clauses such as derived types to also be supported by the use_device_clauses. This is patch 2/2 in a series of patches.
TIFitis
force-pushed
the
users/akash/use-device-lowering-2
branch
from
9cb5045 to
10f575c
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/53/builds/3973 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/72/builds/2941 Here is the relevant piece of the build log for the reference |
TIFitis
added a commit
that referenced
this pull request
Sep 13, 2024
skatrak
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Sep 16, 2024
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 patch updates the use_device_ptr and use_device_addr clauses to use the mapInfoOps for lowering. This allows all the types that are handle by the map clauses such as derived types to also be supported by the use_device_clauses.
This is patch 2/2 in a series of patches.