Skip to content

[OpenACC] Implement 'firstprivate' clause copy lowering #154150

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
erichkeane merged 4 commits into from
Aug 19, 2025
Merged

[OpenACC] Implement 'firstprivate' clause copy lowering #154150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
478331c
[OpenACC] Implement 'firstprivate' clause copy lowering
erichkeane Aug 4, 2025
319e93c
Change signature of EmitAutoVarInit to not take variable, store state
erichkeane Aug 18, 2025
48ae8f9
Do suggested changes by andy
erichkeane Aug 18, 2025
ab1cd21
format
erichkeane Aug 18, 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
22 changes: 13 additions & 9 deletions clang/lib/CIR/CodeGen/CIRGenDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,15 +154,19 @@ void CIRGenFunction::emitAutoVarInit(
initializeWhatIsTechnicallyUninitialized(addr);
LValue lv = makeAddrLValue(addr, type, AlignmentSource::Decl);
emitExprAsInit(init, &d, lv);
// In case lv has uses it means we indeed initialized something
// out of it while trying to build the expression, mark it as such.
mlir::Value val = lv.getAddress().getPointer();
assert(val && "Should have an address");
auto allocaOp = val.getDefiningOp<cir::AllocaOp>();
assert(allocaOp && "Address should come straight out of the alloca");

if (!allocaOp.use_empty())
allocaOp.setInitAttr(mlir::UnitAttr::get(&getMLIRContext()));

if (!emission.wasEmittedAsOffloadClause()) {
// In case lv has uses it means we indeed initialized something
// out of it while trying to build the expression, mark it as such.
mlir::Value val = lv.getAddress().getPointer();
assert(val && "Should have an address");
auto allocaOp = val.getDefiningOp<cir::AllocaOp>();
assert(allocaOp && "Address should come straight out of the alloca");

if (!allocaOp.use_empty())
allocaOp.setInitAttr(mlir::UnitAttr::get(&getMLIRContext()));
}

return;
}

Expand Down
50 changes: 44 additions & 6 deletions clang/lib/CIR/CodeGen/CIRGenFunction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,10 @@ class CIRGenFunction : public CIRGenTypeCache {
/// escaping block.
bool IsEscapingByRef = false;

/// True if the variable was emitted as an offload recipe, and thus doesn't
/// have the same sort of alloca initialization.
bool EmittedAsOffload = false;

mlir::Value NRVOFlag{};

struct Invalid {};
Expand All @@ -482,11 +486,18 @@ class CIRGenFunction : public CIRGenTypeCache {

bool wasEmittedAsGlobal() const { return !Addr.isValid(); }

bool wasEmittedAsOffloadClause() const { return EmittedAsOffload; }

/// Returns the raw, allocated address, which is not necessarily
/// the address of the object itself. It is casted to default
/// address space for address space agnostic languages.
Address getAllocatedAddress() const { return Addr; }

// Changes the stored address for the emission. This function should only
// be used in extreme cases, and isn't required to model normal AST
// initialization/variables.
void setAllocatedAddress(Address A) { Addr = A; }

/// Returns the address of the object within this declaration.
/// Note that this does not chase the forwarding pointer for
/// __block decls.
Expand Down Expand Up @@ -518,12 +529,34 @@ class CIRGenFunction : public CIRGenTypeCache {
symbolTable.insert(vd, addr.getPointer());
}

/// Removes a declaration from the address-relationship. This is a function
/// that shouldn't need to be used except in cases where we're adding/removing
/// things that aren't part of the language-semantics AST.
void removeAddrOfLocalVar(const clang::VarDecl *vd) {
localDeclMap.erase(vd);
}
// A class to allow reverting changes to a var-decl's registration to the
// localDeclMap. This is used in cases where things are being inserted into
// the variable list but don't follow normal lookup/search rules, like in
// OpenACC recipe generation.
class DeclMapRevertingRAII {
Copy link
Contributor

@andykaylor andykaylor Aug 18, 2025

Choose a reason for hiding this comment

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

This seems to have a similar purpose to ParamReferenceReplacerRAII in the incubator. That one requires explicitly adding replacement variables and only removes the explicit mappings. The advantage is that it doesn't require copying the entire map.

In the uses here, do you expect any addresses to be added to the map other than clause parameters?

Copy link
Collaborator Author

@erichkeane erichkeane Aug 18, 2025

Choose a reason for hiding this comment

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

I don't really expect ANYTHING other than the clause parameters, but it seemed less error prone than making the add/remove the same operation. That said, I can probably make this 'less intrusive' than a copy + move by making me just record the decl that I'd like to replace the value of.

Copy link
Contributor

@andykaylor andykaylor Aug 18, 2025

Choose a reason for hiding this comment

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

I could live with it either way, but it would be nice if we could somehow combine this with the coroutine parameter handling when that gets upstreamed.

Copy link
Collaborator Author

@erichkeane erichkeane Aug 19, 2025

Choose a reason for hiding this comment

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

Ping me when that gets to review, and I'll work with the person to combine them. At the moment, I'm a bit too 'simple' for that, but when that goes under review I'll work with the uathor to combine the uses.

CIRGenFunction &cgf;
const VarDecl *vd;
bool shouldDelete = false;
Address oldAddr = Address::invalid();

public:
DeclMapRevertingRAII(CIRGenFunction &cgf, const VarDecl *vd)
: cgf(cgf), vd(vd) {
auto mapItr = cgf.localDeclMap.find(vd);

if (mapItr != cgf.localDeclMap.end())
oldAddr = mapItr->second;
else
shouldDelete = true;
}

~DeclMapRevertingRAII() {
if (shouldDelete)
cgf.localDeclMap.erase(vd);
else
cgf.localDeclMap.insert_or_assign(vd, oldAddr);
}
};

bool shouldNullCheckClassCastValue(const CastExpr *ce);

Expand Down Expand Up @@ -961,6 +994,11 @@ class CIRGenFunction : public CIRGenTypeCache {
void emitAutoVarDecl(const clang::VarDecl &d);

void emitAutoVarCleanups(const AutoVarEmission &emission);
/// Emit the initializer for an allocated variable. If this call is not
/// associated with the call to emitAutoVarAlloca (as the address of the
/// emission is not directly an alloca), the allocatedSeparately parameter can
/// be used to suppress the assertions. However, this should only be used in
/// extreme cases, as it doesn't properly reflect the language/AST.
void emitAutoVarInit(const AutoVarEmission &emission);
void emitAutoVarTypeCleanup(const AutoVarEmission &emission,
clang::QualType::DestructionKind dtorKind);
Expand Down
42 changes: 32 additions & 10 deletions clang/lib/CIR/CodeGen/CIRGenOpenACCClause.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,12 +392,29 @@ class OpenACCClauseCIREmitter final
CIRGenFunction::AutoVarEmission tempDeclEmission,
mlir::acc::FirstprivateRecipeOp recipe, const VarDecl *varRecipe,
const VarDecl *temporary) {
builder.createBlock(&recipe.getCopyRegion(), recipe.getCopyRegion().end(),
{mainOp.getType(), mainOp.getType()}, {loc, loc});
mlir::Block *block = builder.createBlock(
&recipe.getCopyRegion(), recipe.getCopyRegion().end(),
{mainOp.getType(), mainOp.getType()}, {loc, loc});
builder.setInsertionPointToEnd(&recipe.getCopyRegion().back());

// TODO: OpenACC: Implement this copy to actually do something.
mlir::BlockArgument fromArg = block->getArgument(0);
mlir::BlockArgument toArg = block->getArgument(1);

mlir::Type elementTy =
mlir::cast<cir::PointerType>(mainOp.getType()).getPointee();

// Set the address of the emission to be the argument, so that we initialize
// that instead of the variable in the other block.
tempDeclEmission.setAllocatedAddress(
Address{toArg, elementTy, cgf.getContext().getDeclAlign(varRecipe)});
tempDeclEmission.EmittedAsOffload = true;

CIRGenFunction::DeclMapRevertingRAII declMapRAII{cgf, temporary};
cgf.setAddrOfLocalVar(
temporary,
Address{fromArg, elementTy, cgf.getContext().getDeclAlign(varRecipe)});

cgf.emitAutoVarInit(tempDeclEmission);
mlir::acc::YieldOp::create(builder, locEnd);
}

Expand All @@ -417,6 +434,7 @@ class OpenACCClauseCIREmitter final

CIRGenFunction::AutoVarEmission tempDeclEmission{
CIRGenFunction::AutoVarEmission::invalid()};
CIRGenFunction::DeclMapRevertingRAII declMapRAII{cgf, varRecipe};

// Do the 'init' section of the recipe IR, which does an alloca, then the
// initialization (except for firstprivate).
Expand All @@ -425,6 +443,7 @@ class OpenACCClauseCIREmitter final
builder.setInsertionPointToEnd(&recipe.getInitRegion().back());
tempDeclEmission =
cgf.emitAutoVarAlloca(*varRecipe, builder.saveInsertionPoint());

// 'firstprivate' doesn't do its initialization in the 'init' section,
// instead does it in the 'copy' section. SO only do init here.
// 'reduction' appears to use it too (rather than a 'copy' section), so
Expand All @@ -450,16 +469,19 @@ class OpenACCClauseCIREmitter final
mlir::acc::YieldOp::create(builder, locEnd);

if constexpr (std::is_same_v<RecipeTy, mlir::acc::FirstprivateRecipeOp>) {
// TODO: OpenACC: we should have a errorNYI call here if
// !varRecipe->getInit(), but as that generation isn't currently
// implemented, it ends up being too noisy. So when we implement copy-init
// generation both in Sema and here, we should have a diagnostic here.
if (!varRecipe->getInit()) {
// If we don't have any initialization recipe, we failed during Sema to
// initialize this correctly. If we disable the
// Sema::TentativeAnalysisScopes in SemaOpenACC::CreateInitRecipe, it'll
// emit an error to tell us. However, emitting those errors during
// production is a violation of the standard, so we cannot do them.
cgf.cgm.errorNYI(
exprRange, "firstprivate copy-init recipe not properly generated");
}

createFirstprivateRecipeCopy(loc, locEnd, mainOp, tempDeclEmission,
recipe, varRecipe, temporary);
}

// Make sure we cleanup after ourselves here.
cgf.removeAddrOfLocalVar(varRecipe);
}

void createRecipeDestroySection(mlir::Location loc, mlir::Location locEnd,
Expand Down
Loading