Skip to content

[MLIR][OpenMP] Assert on map translation functions, NFC #137199

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 15, 2025
Merged

[MLIR][OpenMP] Assert on map translation functions, NFC #137199

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
59bc1c9
[Flang][MLIR][OpenMP] Improve use_device_* handling
skatrak Apr 11, 2025
c6954b3
[MLIR][OpenMP] Assert on map translation functions, NFC
skatrak Apr 11, 2025
81f0bf4
Merge branch 'main' into users/skatrak/map-rework-02-mark-host-device
skatrak May 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3720,6 +3720,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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

: 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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.


// Map the first segment of our structure
combinedInfo.Types.emplace_back(
isTargetParams
Expand Down Expand Up @@ -3828,6 +3831,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]);
Expand Down Expand Up @@ -3941,6 +3946,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]);

Expand Down Expand Up @@ -3982,6 +3990,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]) {
Expand Down Expand Up @@ -4046,6 +4056,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
Expand All @@ -4057,8 +4070,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();

Expand Down Expand Up @@ -4092,6 +4104,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(
Expand All @@ -4108,6 +4122,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>());
Expand Down Expand Up @@ -4597,6 +4613,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
Expand Down Expand Up @@ -4689,6 +4707,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;
Expand Down
Loading