Skip to content
Open
Changes from all commits
Commits
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
150 changes: 150 additions & 0 deletions clang/lib/CodeGen/CGOpenMPRuntime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9256,6 +9256,156 @@ class MappableExprsHandler {
}
}

/// On a target construct, if there's an implicit map on a struct, or that of
/// this[:], and an explicit map with a member of that struct/class as the
/// base-pointer, we need to make sure that base-pointer is implicitly mapped,
/// to make sure we don't map the full struct/class. For example:
///
/// \code
/// struct S {
/// int dummy[10000];
/// int *p;
/// void f1() {
/// #pragma omp target map(p[0:1])
/// (void)this;
/// }
/// }; S s;
///
/// void f2() {
/// #pragma omp target map(s.p[0:10])
/// (void)s;
/// }
/// \endcode
///
/// Only `this-p` and `s.p` should be mapped in the two cases above.
//
// OpenMP 6.0: 7.9.6 map clause, pg 285
// If a list item with an implicitly determined data-mapping attribute does
// not have any corresponding storage in the device data environment prior to
// a task encountering the construct associated with the map clause, and one
// or more contiguous parts of the original storage are either list items or
// base pointers to list items that are explicitly mapped on the construct,
// only those parts of the original storage will have corresponding storage in
// the device data environment as a result of the map clauses on the
// construct.
void addImplicitMapForAttachPtrBaseIfMemberOfCapturedVD(
const ValueDecl *CapturedVD, MapDataArrayTy &DeclComponentLists,
SmallVectorImpl<
SmallVector<OMPClauseMappableExprCommon::MappableComponent, 8>>
&ComponentVectorStorage) const {
Comment on lines +9291 to +9295
Copy link
Member

Choose a reason for hiding this comment

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

I don't like it make it NFC, this is not NFC at all, and adding a new OMP_MAP_ATTACH too

Copy link
Contributor Author

@abhinavgaba abhinavgaba Oct 1, 2025

Choose a reason for hiding this comment

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

This function is not called in this PR, which is what makes it an NFC. And since we are never adding the ATTACH map-type in this PR, the changes in OMPIRBuilder changes are also never triggered.

The reason to pull these changes out of the parent was just to help with the reviews.

If you don't think it's easier to review this as a subset, then please continue with reviewing the parent #153683, and this one can be closed.

Copy link
Collaborator

@shafik shafik Oct 3, 2025

Choose a reason for hiding this comment

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

I agree, this really stretches the idea of NFC. This is large change, you adding members to a class, changing how other members are initialized etc. There is no loss in removing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR was created after receiving feedback that it would be good to break the parent PR #153684 into smaller patches that can be merged independently. This is one such smaller patch. The NFC tag can be removed that causes concern/confusion.

There is no change in the output IR with this change alone. The parent PR is needed to change the output IR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the NFC tag, and removed the other util and the change to IRBUilder from this PR.

bool IsThisCapture = CapturedVD == nullptr;

for (const auto &ComponentsAndAttachPtr : AttachPtrExprMap) {
OMPClauseMappableExprCommon::MappableExprComponentListRef
ComponentsWithAttachPtr = ComponentsAndAttachPtr.first;
const Expr *AttachPtrExpr = ComponentsAndAttachPtr.second;
if (!AttachPtrExpr)
continue;

const auto *ME = dyn_cast<MemberExpr>(AttachPtrExpr);
if (!ME)
continue;

const Expr *Base = ME->getBase()->IgnoreParenImpCasts();

// If we are handling a "this" capture, then we are looking for
// attach-ptrs of form `this->p`, either explicitly or implicitly.
if (IsThisCapture && !ME->isImplicitCXXThis() && !isa<CXXThisExpr>(Base))
continue;

if (!IsThisCapture && (!isa<DeclRefExpr>(Base) ||
cast<DeclRefExpr>(Base)->getDecl() != CapturedVD))
continue;

// For non-this captures, we are looking for attach-ptrs of form
// `s.p`.
// For non-this captures, we are looking for attach-ptrs like `s.p`.
if (!IsThisCapture && (ME->isArrow() || !isa<DeclRefExpr>(Base) ||
cast<DeclRefExpr>(Base)->getDecl() != CapturedVD))
continue;

// Check if we have an existing map on either:
// this[:], s, this->p, or s.p, in which case, we don't need to add
// an implicit one for the attach-ptr s.p/this->p.
bool FoundExistingMap = false;
for (const MapData &ExistingL : DeclComponentLists) {
OMPClauseMappableExprCommon::MappableExprComponentListRef
ExistingComponents = std::get<0>(ExistingL);

if (ExistingComponents.empty())
continue;

// First check if we have a map like map(this->p) or map(s.p).
const auto &FirstComponent = ExistingComponents.front();
const Expr *FirstExpr = FirstComponent.getAssociatedExpression();

if (!FirstExpr)
continue;

// First check if we have a map like map(this->p) or map(s.p).
if (AttachPtrComparator.areEqual(FirstExpr, AttachPtrExpr)) {
FoundExistingMap = true;
break;
}

// Check if we have a map like this[0:1]
if (IsThisCapture) {
if (const auto *OASE = dyn_cast<ArraySectionExpr>(FirstExpr)) {
if (isa<CXXThisExpr>(OASE->getBase()->IgnoreParenImpCasts())) {
FoundExistingMap = true;
break;
}
}
continue;
}

// When the attach-ptr is something like `s.p`, check if
// `s` itself is mapped explicitly.
if (const auto *DRE = dyn_cast<DeclRefExpr>(FirstExpr)) {
if (DRE->getDecl() == CapturedVD) {
FoundExistingMap = true;
break;
}
}
}

if (FoundExistingMap)
continue;

// If no base map is found, we need to create an implicit map for the
// attach-pointer expr.

ComponentVectorStorage.emplace_back();
auto &AttachPtrComponents = ComponentVectorStorage.back();

static const OpenMPMapModifierKind Unknown = OMPC_MAP_MODIFIER_unknown;
bool SeenAttachPtrComponent = false;
// For creating a map on the attach-ptr `s.p/this->p`, we copy all
// components from the component-list which has `s.p/this->p`
// as the attach-ptr, starting from the component which matches
// `s.p/this->p`. This way, we'll have component-lists of
// `s.p` -> `s`, and `this->p` -> `this`.
for (size_t i = 0; i < ComponentsWithAttachPtr.size(); ++i) {
const auto &Component = ComponentsWithAttachPtr[i];
const Expr *ComponentExpr = Component.getAssociatedExpression();

if (!SeenAttachPtrComponent && ComponentExpr != AttachPtrExpr)
continue;
SeenAttachPtrComponent = true;

AttachPtrComponents.emplace_back(Component.getAssociatedExpression(),
Component.getAssociatedDeclaration(),
Component.isNonContiguous());
}
assert(!AttachPtrComponents.empty() &&
"Could not populate component-lists for mapping attach-ptr");

DeclComponentLists.emplace_back(
AttachPtrComponents, OMPC_MAP_tofrom, Unknown,
/*IsImplicit=*/true, /*mapper=*/nullptr, AttachPtrExpr);
}
}

/// For a capture that has an associated clause, generate the base pointers,
/// section pointers, sizes, map types, and mappers (all included in
/// \a CurCaptureVarInfo).
Expand Down