[MLIR][OpenMP] Assert on map translation functions, NFC #137199
Conversation
skatrak
requested review from
ergawy,
tblah,
kiranchandramohan,
agozillon,
TIFitis and
bhandarkar-pranav
|
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-mlir Author: Sergio Afonso (skatrak) ChangesThis patch adds assertions to map-related MLIR to LLVM IR translation functions and utils to explicitly document whether they are intended for host or device compilation only. Over time, map-related handling has increased in complexity. This is compounded by the fact that some handling is device-specific and some is host-specific. By explicitly asserting on these functions on the expected compilation pass, the flow should become slighlty easier to follow. Full diff: https://github.com/llvm/llvm-project/pull/137199.diff 1 Files Affected:
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 52aa1fbfab2c1..6d80c66e3596e 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -3563,6 +3563,9 @@ static llvm::omp::OpenMPOffloadMappingFlags mapParentWithMembers(
LLVM::ModuleTranslation &moduleTranslation, llvm::IRBuilderBase &builder,
llvm::OpenMPIRBuilder &ompBuilder, DataLayout &dl, MapInfosTy &combinedInfo,
MapInfoData &mapData, uint64_t mapDataIndex, bool isTargetParams) {
+ assert(!ompBuilder.Config.isTargetDevice() &&
+ "function only supported for host device codegen");
+
// Map the first segment of our structure
combinedInfo.Types.emplace_back(
isTargetParams
@@ -3671,6 +3674,8 @@ static void processMapMembersWithParent(
llvm::OpenMPIRBuilder &ompBuilder, DataLayout &dl, MapInfosTy &combinedInfo,
MapInfoData &mapData, uint64_t mapDataIndex,
llvm::omp::OpenMPOffloadMappingFlags memberOfFlag) {
+ assert(!ompBuilder.Config.isTargetDevice() &&
+ "function only supported for host device codegen");
auto parentClause =
llvm::cast<omp::MapInfoOp>(mapData.MapClause[mapDataIndex]);
@@ -3784,6 +3789,9 @@ static void processMapWithMembersOf(LLVM::ModuleTranslation &moduleTranslation,
DataLayout &dl, MapInfosTy &combinedInfo,
MapInfoData &mapData, uint64_t mapDataIndex,
bool isTargetParams) {
+ assert(!ompBuilder.Config.isTargetDevice() &&
+ "function only supported for host device codegen");
+
auto parentClause =
llvm::cast<omp::MapInfoOp>(mapData.MapClause[mapDataIndex]);
@@ -3825,6 +3833,8 @@ static void
createAlteredByCaptureMap(MapInfoData &mapData,
LLVM::ModuleTranslation &moduleTranslation,
llvm::IRBuilderBase &builder) {
+ assert(!moduleTranslation.getOpenMPBuilder()->Config.isTargetDevice() &&
+ "function only supported for host device codegen");
for (size_t i = 0; i < mapData.MapClause.size(); ++i) {
// if it's declare target, skip it, it's handled separately.
if (!mapData.IsDeclareTarget[i]) {
@@ -3889,6 +3899,9 @@ static void genMapInfos(llvm::IRBuilderBase &builder,
LLVM::ModuleTranslation &moduleTranslation,
DataLayout &dl, MapInfosTy &combinedInfo,
MapInfoData &mapData, bool isTargetParams = false) {
+ assert(!moduleTranslation.getOpenMPBuilder()->Config.isTargetDevice() &&
+ "function only supported for host device codegen");
+
// 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
@@ -3900,8 +3913,7 @@ static void genMapInfos(llvm::IRBuilderBase &builder,
// kernel arg structure. It primarily becomes relevant in cases like
// bycopy, or byref range'd arrays. In the default case, we simply
// pass thee pointer byref as both basePointer and pointer.
- if (!moduleTranslation.getOpenMPBuilder()->Config.isTargetDevice())
- createAlteredByCaptureMap(mapData, moduleTranslation, builder);
+ createAlteredByCaptureMap(mapData, moduleTranslation, builder);
llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
@@ -3935,6 +3947,8 @@ emitUserDefinedMapper(Operation *declMapperOp, llvm::IRBuilderBase &builder,
static llvm::Expected<llvm::Function *>
getOrCreateUserDefinedMapperFunc(Operation *op, llvm::IRBuilderBase &builder,
LLVM::ModuleTranslation &moduleTranslation) {
+ assert(!moduleTranslation.getOpenMPBuilder()->Config.isTargetDevice() &&
+ "function only supported for host device codegen");
auto declMapperOp = cast<omp::DeclareMapperOp>(op);
std::string mapperFuncName =
moduleTranslation.getOpenMPBuilder()->createPlatformSpecificName(
@@ -3951,6 +3965,8 @@ static llvm::Expected<llvm::Function *>
emitUserDefinedMapper(Operation *op, llvm::IRBuilderBase &builder,
LLVM::ModuleTranslation &moduleTranslation,
llvm::StringRef mapperFuncName) {
+ assert(!moduleTranslation.getOpenMPBuilder()->Config.isTargetDevice() &&
+ "function only supported for host device codegen");
auto declMapperOp = cast<omp::DeclareMapperOp>(op);
auto declMapperInfoOp = declMapperOp.getDeclareMapperInfo();
DataLayout dl = DataLayout(declMapperOp->getParentOfType<ModuleOp>());
@@ -4440,6 +4456,8 @@ static void
handleDeclareTargetMapVar(MapInfoData &mapData,
LLVM::ModuleTranslation &moduleTranslation,
llvm::IRBuilderBase &builder, llvm::Function *func) {
+ assert(moduleTranslation.getOpenMPBuilder()->Config.isTargetDevice() &&
+ "function only supported for target device codegen");
for (size_t i = 0; i < mapData.MapClause.size(); ++i) {
// In the case of declare target mapped variables, the basePointer is
// the reference pointer generated by the convertDeclareTargetAttr
@@ -4532,6 +4550,8 @@ createDeviceArgumentAccessor(MapInfoData &mapData, llvm::Argument &arg,
LLVM::ModuleTranslation &moduleTranslation,
llvm::IRBuilderBase::InsertPoint allocaIP,
llvm::IRBuilderBase::InsertPoint codeGenIP) {
+ assert(ompBuilder.Config.isTargetDevice() &&
+ "function only supported for target device codegen");
builder.restoreIP(allocaIP);
omp::VariableCaptureKind capture = omp::VariableCaptureKind::ByRef;
|
|
@llvm/pr-subscribers-mlir-openmp Author: Sergio Afonso (skatrak) ChangesThis patch adds assertions to map-related MLIR to LLVM IR translation functions and utils to explicitly document whether they are intended for host or device compilation only. Over time, map-related handling has increased in complexity. This is compounded by the fact that some handling is device-specific and some is host-specific. By explicitly asserting on these functions on the expected compilation pass, the flow should become slighlty easier to follow. Full diff: https://github.com/llvm/llvm-project/pull/137199.diff 1 Files Affected:
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 52aa1fbfab2c1..6d80c66e3596e 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -3563,6 +3563,9 @@ static llvm::omp::OpenMPOffloadMappingFlags mapParentWithMembers(
LLVM::ModuleTranslation &moduleTranslation, llvm::IRBuilderBase &builder,
llvm::OpenMPIRBuilder &ompBuilder, DataLayout &dl, MapInfosTy &combinedInfo,
MapInfoData &mapData, uint64_t mapDataIndex, bool isTargetParams) {
+ assert(!ompBuilder.Config.isTargetDevice() &&
+ "function only supported for host device codegen");
+
// Map the first segment of our structure
combinedInfo.Types.emplace_back(
isTargetParams
@@ -3671,6 +3674,8 @@ static void processMapMembersWithParent(
llvm::OpenMPIRBuilder &ompBuilder, DataLayout &dl, MapInfosTy &combinedInfo,
MapInfoData &mapData, uint64_t mapDataIndex,
llvm::omp::OpenMPOffloadMappingFlags memberOfFlag) {
+ assert(!ompBuilder.Config.isTargetDevice() &&
+ "function only supported for host device codegen");
auto parentClause =
llvm::cast<omp::MapInfoOp>(mapData.MapClause[mapDataIndex]);
@@ -3784,6 +3789,9 @@ static void processMapWithMembersOf(LLVM::ModuleTranslation &moduleTranslation,
DataLayout &dl, MapInfosTy &combinedInfo,
MapInfoData &mapData, uint64_t mapDataIndex,
bool isTargetParams) {
+ assert(!ompBuilder.Config.isTargetDevice() &&
+ "function only supported for host device codegen");
+
auto parentClause =
llvm::cast<omp::MapInfoOp>(mapData.MapClause[mapDataIndex]);
@@ -3825,6 +3833,8 @@ static void
createAlteredByCaptureMap(MapInfoData &mapData,
LLVM::ModuleTranslation &moduleTranslation,
llvm::IRBuilderBase &builder) {
+ assert(!moduleTranslation.getOpenMPBuilder()->Config.isTargetDevice() &&
+ "function only supported for host device codegen");
for (size_t i = 0; i < mapData.MapClause.size(); ++i) {
// if it's declare target, skip it, it's handled separately.
if (!mapData.IsDeclareTarget[i]) {
@@ -3889,6 +3899,9 @@ static void genMapInfos(llvm::IRBuilderBase &builder,
LLVM::ModuleTranslation &moduleTranslation,
DataLayout &dl, MapInfosTy &combinedInfo,
MapInfoData &mapData, bool isTargetParams = false) {
+ assert(!moduleTranslation.getOpenMPBuilder()->Config.isTargetDevice() &&
+ "function only supported for host device codegen");
+
// 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
@@ -3900,8 +3913,7 @@ static void genMapInfos(llvm::IRBuilderBase &builder,
// kernel arg structure. It primarily becomes relevant in cases like
// bycopy, or byref range'd arrays. In the default case, we simply
// pass thee pointer byref as both basePointer and pointer.
- if (!moduleTranslation.getOpenMPBuilder()->Config.isTargetDevice())
- createAlteredByCaptureMap(mapData, moduleTranslation, builder);
+ createAlteredByCaptureMap(mapData, moduleTranslation, builder);
llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
@@ -3935,6 +3947,8 @@ emitUserDefinedMapper(Operation *declMapperOp, llvm::IRBuilderBase &builder,
static llvm::Expected<llvm::Function *>
getOrCreateUserDefinedMapperFunc(Operation *op, llvm::IRBuilderBase &builder,
LLVM::ModuleTranslation &moduleTranslation) {
+ assert(!moduleTranslation.getOpenMPBuilder()->Config.isTargetDevice() &&
+ "function only supported for host device codegen");
auto declMapperOp = cast<omp::DeclareMapperOp>(op);
std::string mapperFuncName =
moduleTranslation.getOpenMPBuilder()->createPlatformSpecificName(
@@ -3951,6 +3965,8 @@ static llvm::Expected<llvm::Function *>
emitUserDefinedMapper(Operation *op, llvm::IRBuilderBase &builder,
LLVM::ModuleTranslation &moduleTranslation,
llvm::StringRef mapperFuncName) {
+ assert(!moduleTranslation.getOpenMPBuilder()->Config.isTargetDevice() &&
+ "function only supported for host device codegen");
auto declMapperOp = cast<omp::DeclareMapperOp>(op);
auto declMapperInfoOp = declMapperOp.getDeclareMapperInfo();
DataLayout dl = DataLayout(declMapperOp->getParentOfType<ModuleOp>());
@@ -4440,6 +4456,8 @@ static void
handleDeclareTargetMapVar(MapInfoData &mapData,
LLVM::ModuleTranslation &moduleTranslation,
llvm::IRBuilderBase &builder, llvm::Function *func) {
+ assert(moduleTranslation.getOpenMPBuilder()->Config.isTargetDevice() &&
+ "function only supported for target device codegen");
for (size_t i = 0; i < mapData.MapClause.size(); ++i) {
// In the case of declare target mapped variables, the basePointer is
// the reference pointer generated by the convertDeclareTargetAttr
@@ -4532,6 +4550,8 @@ createDeviceArgumentAccessor(MapInfoData &mapData, llvm::Argument &arg,
LLVM::ModuleTranslation &moduleTranslation,
llvm::IRBuilderBase::InsertPoint allocaIP,
llvm::IRBuilderBase::InsertPoint codeGenIP) {
+ assert(ompBuilder.Config.isTargetDevice() &&
+ "function only supported for target device codegen");
builder.restoreIP(allocaIP);
omp::VariableCaptureKind capture = omp::VariableCaptureKind::ByRef;
|
|
This PR is based on the following analysis of the current state of OpenMP map translation to LLVM IR. If there are any issues with this PR, they are likely easier to spot on this graph: |
skatrak
force-pushed
the
users/skatrak/map-rework-01-use-device
branch
from
de00b77 to
beb8430
Compare
skatrak
force-pushed
the
users/skatrak/map-rework-02-mark-host-device
branch
from
b70cbfa to
6ea52bf
Compare
| @@ -3623,6 +3623,9 @@ static llvm::omp::OpenMPOffloadMappingFlags mapParentWithMembers( | |||
| LLVM::ModuleTranslation &moduleTranslation, llvm::IRBuilderBase &builder, | |||
| llvm::OpenMPIRBuilder &ompBuilder, DataLayout &dl, MapInfosTy &combinedInfo, | |||
| MapInfoData &mapData, uint64_t mapDataIndex, bool isTargetParams) { | |||
| assert(!ompBuilder.Config.isTargetDevice() && | |||
| "function only supported for host device codegen"); | |||
There was a problem hiding this comment.
: The wording would be that much clearer if you didn't use the word device here at all.
"function only supported for host codegen" or "function only supported for host side codegen"
I know that here you the two terms you are using that are opposite to each other in meaning are "host device" and "target device", but when an assert is hit only one of them will be seen. If it is "target device" that's fine because target, target device and device are sort of used interchangeably (in my experience so far). However, I have come across ample usage of just "device" to mean target only. So, host device can be confusing.
However, I do also recognize that "hostdevice" is used in other places, in at least the runtime. So, if "host device" is more prevalent, then ignore this comment entirely.
There was a problem hiding this comment.
Ok, i just saw your diagram and I see "host device" is something. I'll just wait for you to correct me at this point ;-)
There was a problem hiding this comment.
Yes, I do agree that "host device" seems like a rather contradictory term, since we generally just talk about "host" as the CPU and "device" as whatever we're offloading to. The reason of using these terms is to align with OpenMP terminology (5.2 spec, Section 1.2.1):
host device The device on which the OpenMP program begins execution.
target device A device with respect to which the current device performs an operation, as specified by a device construct or an OpenMP device memory routine.
This is also why the -fopenmp-is-target-device flag is called that and not -fopenmp-is-device (which used to be its name).
There was a problem hiding this comment.
Yes, I do agree that "host device" seems like a rather contradictory term, since we generally just talk about "host" as the CPU and "device" as whatever we're offloading to. The reason of using these terms is to align with OpenMP terminology (5.2 spec, Section 1.2.1):
host device The device on which the OpenMP program begins execution.
target device A device with respect to which the current device performs an operation, as specified by a device construct or an OpenMP device memory routine.This is also why the
-fopenmp-is-target-deviceflag is called that and not-fopenmp-is-device(which used to be its name).
Thank you, good to know. So in this world every thing is a device and some devices are hosts and some are targets meant to be offloaded to.
This patch updates MLIR op verifiers for operations taking arguments that must
always be defined by an `omp.map.info` operation to check this requirement.
It also modifies Flang lowering for `use_device_{addr, ptr}`, as well as the
custom MLIR printer and parser for these clauses, to support initializing it to
`OMP_MAP_RETURN_PARAM` and represent this in the MLIR representation as
`return_param`. This internal mapping flag is what eventually is used for
variables passed via these clauses into the target region when translating to
LLVM IR, so making it explicit in Flang and MLIR removes an inconsistency in
the current representation.
skatrak
force-pushed
the
users/skatrak/map-rework-01-use-device
branch
from
beb8430 to
59bc1c9
Compare
This patch adds assertions to map-related MLIR to LLVM IR translation functions and utils to explicitly document whether they are intended for host or device compilation only. Over time, map-related handling has increased in complexity. This is compounded by the fact that some handling is device-specific and some is host-specific. By explicitly asserting on these functions on the expected compilation pass, the flow should become slighlty easier to follow.
skatrak
force-pushed
the
users/skatrak/map-rework-02-mark-host-device
branch
from
6ea52bf to
c6954b3
Compare
This patch adds assertions to map-related MLIR to LLVM IR translation functions and utils to explicitly document whether they are intended for host or device compilation only. Over time, map-related handling has increased in complexity. This is compounded by the fact that some handling is device-specific and some is host-specific. By explicitly asserting on these functions on the expected compilation pass, the flow should become slighlty easier to follow.
This patch adds assertions to map-related MLIR to LLVM IR translation functions and utils to explicitly document whether they are intended for host or device compilation only.
Over time, map-related handling has increased in complexity. This is compounded by the fact that some handling is device-specific and some is host-specific. By explicitly asserting on these functions on the expected compilation pass, the flow should become slighlty easier to follow.