Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
6 changes: 3 additions & 3 deletions clang/lib/CIR/CodeGen/CIRGenOpenACCClause.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1005,7 +1005,7 @@ class OpenACCClauseCIREmitter final
/*temporary=*/nullptr, OpenACCReductionOperator::Invalid,
Decl::castToDeclContext(cgf.curFuncDecl), opInfo.origType,
opInfo.bounds.size(), opInfo.boundTypes, opInfo.baseType,
privateOp);
privateOp, /*reductionCombinerRecipe=*/{});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
privateOp, /*reductionCombinerRecipe=*/{});
privateOp, /*reductionCombinerRecipes=*/{});

// TODO: OpenACC: The dialect is going to change in the near future to
// have these be on a different operation, so when that changes, we
// probably need to change these here.
Expand Down Expand Up @@ -1046,7 +1046,7 @@ class OpenACCClauseCIREmitter final
OpenACCReductionOperator::Invalid,
Decl::castToDeclContext(cgf.curFuncDecl), opInfo.origType,
opInfo.bounds.size(), opInfo.boundTypes, opInfo.baseType,
firstPrivateOp);
firstPrivateOp, /*reductionCombinerRecipe=*/{});

// TODO: OpenACC: The dialect is going to change in the near future to
// have these be on a different operation, so when that changes, we
Expand Down Expand Up @@ -1088,7 +1088,7 @@ class OpenACCClauseCIREmitter final
/*temporary=*/nullptr, clause.getReductionOp(),
Decl::castToDeclContext(cgf.curFuncDecl), opInfo.origType,
opInfo.bounds.size(), opInfo.boundTypes, opInfo.baseType,
reductionOp);
reductionOp, varRecipe.CombinerRecipes);

operation.addReduction(builder.getContext(), reductionOp, recipe);
}
Expand Down
130 changes: 127 additions & 3 deletions clang/lib/CIR/CodeGen/CIRGenOpenACCRecipe.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -527,16 +527,140 @@ void OpenACCRecipeBuilderBase::createFirstprivateRecipeCopy(
// doesn't restore it aftewards.
void OpenACCRecipeBuilderBase::createReductionRecipeCombiner(
mlir::Location loc, mlir::Location locEnd, mlir::Value mainOp,
mlir::acc::ReductionRecipeOp recipe, size_t numBounds) {
mlir::acc::ReductionRecipeOp recipe, size_t numBounds, QualType origType,
llvm::ArrayRef<OpenACCReductionRecipe::CombinerRecipe> combinerRecipes) {
mlir::Block *block =
createRecipeBlock(recipe.getCombinerRegion(), mainOp.getType(), loc,
numBounds, /*isInit=*/false);
builder.setInsertionPointToEnd(&recipe.getCombinerRegion().back());
CIRGenFunction::LexicalScope ls(cgf, loc, block);

mlir::BlockArgument lhsArg = block->getArgument(0);
mlir::Value lhsArg = block->getArgument(0);
mlir::Value rhsArg = block->getArgument(1);
llvm::MutableArrayRef<mlir::BlockArgument> boundsRange =
block->getArguments().drop_front(2);

if (llvm::any_of(combinerRecipes, [](auto &r) { return r.Op == nullptr; })) {
cgf.cgm.errorNYI(loc, "OpenACC Reduction combiner not generated");
mlir::acc::YieldOp::create(builder, locEnd, block->getArgument(0));
return;
}

// apply the bounds so that we can get our bounds emitted correctly.
for (mlir::BlockArgument boundArg : llvm::reverse(boundsRange))
std::tie(lhsArg, rhsArg) =
createBoundsLoop(lhsArg, rhsArg, boundArg, loc, /*inverse=*/false);

// Emitter for when we know this isn't a struct or array we have to loop
// through. This should work for the 'field' once the get-element call has
// been made.
auto emitSingleCombiner =
[&](mlir::Value lhsArg, mlir::Value rhsArg,
const OpenACCReductionRecipe::CombinerRecipe &combiner) {
mlir::Type elementTy =
mlir::cast<cir::PointerType>(lhsArg.getType()).getPointee();
CIRGenFunction::DeclMapRevertingRAII declMapRAIILhs{cgf, combiner.LHS};
cgf.setAddrOfLocalVar(
combiner.LHS, Address{lhsArg, elementTy,
cgf.getContext().getDeclAlign(combiner.LHS)});
CIRGenFunction::DeclMapRevertingRAII declMapRAIIRhs{cgf, combiner.RHS};
cgf.setAddrOfLocalVar(
combiner.RHS, Address{rhsArg, elementTy,
cgf.getContext().getDeclAlign(combiner.RHS)});

[[maybe_unused]] mlir::LogicalResult stmtRes =
cgf.emitStmt(combiner.Op, /*useCurrentScope=*/true);
};

// Emitter for when we know this is either a non-array or element of an array
// (which also shouldn't be an array type?). This function should generate the
// loop to do this on each individual array or struct element (if necessary).
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the comment. If we know this is a non-array, what does the part about a loop to do this on each individual array element mean?

Copy link
Collaborator Author

@erichkeane erichkeane Oct 13, 2025

Choose a reason for hiding this comment

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

Ah, yeah, this isn't worded great. It doesn't generate a loop as you see, it should generate the BODY of said loop (just the initialization). In the case of a struct type, it should either call the operator or initialize each individual element.

Once I've solved the Sema issue, I'll reword this in a followup.

EDIT: Where 'followup' is still in this review, just a new commit on this review... you know what I mean, blech :D

auto emitCombiner = [&](mlir::Value lhsArg, mlir::Value rhsArg, QualType Ty) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
auto emitCombiner = [&](mlir::Value lhsArg, mlir::Value rhsArg, QualType Ty) {
auto emitCombiner = [&](mlir::Value lhsArg, mlir::Value rhsArg, QualType ty) {

if (const auto *RD = Ty->getAsRecordDecl()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (const auto *RD = Ty->getAsRecordDecl()) {
if (const auto *rd = Ty->getAsRecordDecl()) {

if (combinerRecipes.size() == 1 &&
cgf.getContext().hasSameType(Ty, combinerRecipes[0].LHS->getType())) {
// If this is a 'top level' operator on the type we can just emit this
// as a simple one.
emitSingleCombiner(lhsArg, rhsArg, combinerRecipes[0]);
} else {
// else we have to handle each individual field after after a
// get-element.
for (const auto &[field, combiner] :
llvm::zip_equal(RD->fields(), combinerRecipes)) {
mlir::Type fieldType = cgf.convertType(field->getType());
auto fieldPtr = cir::PointerType::get(fieldType);

mlir::Value lhsField = builder.createGetMember(
loc, fieldPtr, lhsArg, field->getName(), field->getFieldIndex());
mlir::Value rhsField = builder.createGetMember(
loc, fieldPtr, rhsArg, field->getName(), field->getFieldIndex());

emitSingleCombiner(lhsField, rhsField, combiner);
}
}

} else {
// if this is a single-thing (because we should know this isn't an array,
// as Sema wouldn't let us get here), we can just do a normal emit call.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe assert that it's not an array?

emitSingleCombiner(lhsArg, rhsArg, combinerRecipes[0]);
}
};

if (const auto *cat = cgf.getContext().getAsConstantArrayType(origType)) {
// If we're in an array, we have to emit the combiner for each element of
// the array.
auto itrTy = mlir::cast<cir::IntType>(cgf.PtrDiffTy);
auto itrPtrTy = cir::PointerType::get(itrTy);

mlir::Value zero =
builder.getConstInt(loc, mlir::cast<cir::IntType>(cgf.PtrDiffTy), 0);
mlir::Value itr =
cir::AllocaOp::create(builder, loc, itrPtrTy, itrTy, "itr",
cgf.cgm.getSize(cgf.getPointerAlign()));
builder.CIRBaseBuilderTy::createStore(loc, zero, itr);

builder.setInsertionPointAfter(builder.createFor(
loc,
/*condBuilder=*/
[&](mlir::OpBuilder &b, mlir::Location loc) {
auto loadItr = cir::LoadOp::create(builder, loc, {itr});
mlir::Value arraySize = builder.getConstInt(
loc, mlir::cast<cir::IntType>(cgf.PtrDiffTy), cat->getZExtSize());
auto cmp = builder.createCompare(loc, cir::CmpOpKind::lt, loadItr,
arraySize);
builder.createCondition(cmp);
},
/*bodyBuilder=*/
[&](mlir::OpBuilder &b, mlir::Location loc) {
auto loadItr = cir::LoadOp::create(builder, loc, {itr});
auto lhsElt = builder.getArrayElement(
loc, loc, lhsArg, cgf.convertType(cat->getElementType()), loadItr,
/*shouldDecay=*/true);
auto rhsElt = builder.getArrayElement(
loc, loc, rhsArg, cgf.convertType(cat->getElementType()), loadItr,
/*shouldDecay=*/true);

emitCombiner(lhsElt, rhsElt, cat->getElementType());
builder.createYield(loc);
},
/*stepBuilder=*/
[&](mlir::OpBuilder &b, mlir::Location loc) {
auto loadItr = cir::LoadOp::create(builder, loc, {itr});
auto inc = cir::UnaryOp::create(builder, loc, loadItr.getType(),
cir::UnaryOpKind::Inc, loadItr);
builder.CIRBaseBuilderTy::createStore(loc, inc, itr);
builder.createYield(loc);
}));

mlir::acc::YieldOp::create(builder, locEnd, lhsArg);
} else if (origType->isArrayType()) {
cgf.cgm.errorNYI(loc,
"OpenACC Reduction combiner non-constant array recipe");
} else {
emitCombiner(lhsArg, rhsArg, origType);
}

builder.setInsertionPointToEnd(&recipe.getCombinerRegion().back());
mlir::acc::YieldOp::create(builder, locEnd, block->getArgument(0));
}

} // namespace clang::CIRGen
15 changes: 9 additions & 6 deletions clang/lib/CIR/CodeGen/CIRGenOpenACCRecipe.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@ class OpenACCRecipeBuilderBase {
// that this function is not 'insertion point' clean, in that it alters the
// insertion point to be inside of the 'combiner' section of the recipe, but
// doesn't restore it aftewards.
void createReductionRecipeCombiner(mlir::Location loc, mlir::Location locEnd,
mlir::Value mainOp,
mlir::acc::ReductionRecipeOp recipe,
size_t numBounds);
void createReductionRecipeCombiner(
mlir::Location loc, mlir::Location locEnd, mlir::Value mainOp,
mlir::acc::ReductionRecipeOp recipe, size_t numBounds, QualType origType,
llvm::ArrayRef<OpenACCReductionRecipe::CombinerRecipe> combinerRecipes);

void createInitRecipe(mlir::Location loc, mlir::Location locEnd,
SourceRange exprRange, mlir::Value mainOp,
Expand Down Expand Up @@ -169,7 +169,9 @@ class OpenACCRecipeBuilder : OpenACCRecipeBuilderBase {
const Expr *varRef, const VarDecl *varRecipe, const VarDecl *temporary,
OpenACCReductionOperator reductionOp, DeclContext *dc, QualType origType,
size_t numBounds, llvm::ArrayRef<QualType> boundTypes, QualType baseType,
mlir::Value mainOp) {
mlir::Value mainOp,
llvm::ArrayRef<OpenACCReductionRecipe::CombinerRecipe>
reductionCombinerRecipes) {
assert(!varRecipe->getType()->isSpecificBuiltinType(
BuiltinType::ArraySection) &&
"array section shouldn't make it to recipe creation");
Expand Down Expand Up @@ -208,7 +210,8 @@ class OpenACCRecipeBuilder : OpenACCRecipeBuilderBase {
createInitRecipe(loc, locEnd, varRef->getSourceRange(), mainOp,
recipe.getInitRegion(), numBounds, boundTypes, varRecipe,
origType, /*emitInitExpr=*/true);
createReductionRecipeCombiner(loc, locEnd, mainOp, recipe, numBounds);
createReductionRecipeCombiner(loc, locEnd, mainOp, recipe, numBounds,
origType, reductionCombinerRecipes);
} else {
static_assert(std::is_same_v<RecipeTy, mlir::acc::FirstprivateRecipeOp>);
createInitRecipe(loc, locEnd, varRef->getSourceRange(), mainOp,
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Sema/SemaOpenACCClause.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1924,7 +1924,7 @@ bool SemaOpenACC::CheckReductionVarType(Expr *VarExpr) {
// off here. This will result in CurType being the actual 'type' of the
// expression, which is what we are looking to check.
QualType CurType = isa<ArraySectionExpr>(VarExpr)
? ArraySectionExpr::getBaseOriginalType(VarExpr)
? cast<ArraySectionExpr>(VarExpr)->getElementType()
: VarExpr->getType();

// This can happen when we have a dependent type in an array element that the
Expand Down
Loading