Skip to content

[OpenACC][CIR] Reduction combiner lowering for min/max #163656

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 2 commits into from
Oct 20, 2025
Merged

[OpenACC][CIR] Reduction combiner lowering for min/max #163656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de686b7
[OpenACC][CIR] Reduction combiner lowering for min/max
erichkeane Oct 10, 2025
6e6aafe
Merge branch 'main' into ReductionCombinerMinMax
erichkeane Oct 20, 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
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13676,6 +13676,9 @@ def err_acc_reduction_recipe_no_op
"not have a valid operation available">;
def note_acc_reduction_recipe_noop_field
: Note<"while forming combiner for compound type %0">;
def note_acc_reduction_combiner_forming
: Note<"while forming %select{|binary operator '%1'|conditional "
"operator|final assignment operator}0">;

// AMDGCN builtins diagnostics
def err_amdgcn_load_lds_size_invalid_value : Error<"invalid size value">;
Expand Down
7 changes: 5 additions & 2 deletions clang/lib/CIR/CodeGen/CIRGenOpenACCRecipe.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,15 +590,18 @@ void OpenACCRecipeBuilderBase::createReductionRecipeCombiner(
} else {
// else we have to handle each individual field after after a
// get-element.
const CIRGenRecordLayout &layout =
cgf.cgm.getTypes().getCIRGenRecordLayout(rd);
for (const auto &[field, combiner] :
llvm::zip_equal(rd->fields(), combinerRecipes)) {
mlir::Type fieldType = cgf.convertType(field->getType());
auto fieldPtr = cir::PointerType::get(fieldType);
unsigned fieldIndex = layout.getCIRFieldNo(field);

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

emitSingleCombiner(lhsField, rhsField, combiner);
}
Expand Down
93 changes: 80 additions & 13 deletions clang/lib/Sema/SemaOpenACC.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2996,6 +2996,8 @@ bool SemaOpenACC::CreateReductionCombinerRecipe(

case OpenACCReductionOperator::Max:
case OpenACCReductionOperator::Min:
BinOp = BinaryOperatorKind::BO_LT;
break;
case OpenACCReductionOperator::And:
case OpenACCReductionOperator::Or:
// We just want a 'NYI' error in the backend, so leave an empty combiner
Expand All @@ -3011,26 +3013,80 @@ bool SemaOpenACC::CreateReductionCombinerRecipe(

assert(!VarTy->isArrayType() && "Only 1 level of array allowed");

enum class CombinerFailureKind {
None = 0,
BinOp = 1,
Conditional = 2,
Assignment = 3,
};

auto genCombiner = [&, this](DeclRefExpr *LHSDRE, DeclRefExpr *RHSDRE)
-> std::pair<ExprResult, CombinerFailureKind> {
ExprResult BinOpRes =
SemaRef.BuildBinOp(SemaRef.getCurScope(), Loc, BinOp, LHSDRE, RHSDRE,
/*ForFoldExpr=*/false);
switch (ReductionOperator) {
case OpenACCReductionOperator::Addition:
case OpenACCReductionOperator::Multiplication:
case OpenACCReductionOperator::BitwiseAnd:
case OpenACCReductionOperator::BitwiseOr:
case OpenACCReductionOperator::BitwiseXOr:
// These 5 are simple and are being done as compound operators, so we can
// immediately quit here.
return {BinOpRes, BinOpRes.isUsable() ? CombinerFailureKind::None
: CombinerFailureKind::BinOp};
case OpenACCReductionOperator::Max:
case OpenACCReductionOperator::Min: {
// These are done as:
// LHS = (LHS < RHS) ? LHS : RHS; and LHS = (LHS < RHS) ? RHS : LHS;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the spec say anything about how NaN should be handled for min and max with floating point types? I think this implementation will have problems with that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, NaN isn't mentioned, just that I am allowed to use assignment and less-than for the purposes of this. I suspect that NaN are just not noticed/cared about?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. NaN and min/max are notoriously difficult to mix, which is why LLVM has four sets of min/max intrinsics. It's probably a good idea for the standard to clarify expectations explicitly, but if it says it's OK to use less than, I would interpret that as saying that NaN produces at least unspecified if not undefined behavior.

//
// The BinOpRes should have been created with the less-than, so we just
// have to build the conditional and assignment.
if (!BinOpRes.isUsable())
return {BinOpRes, CombinerFailureKind::BinOp};

// Create the correct conditional operator, swapping the results
// (true/false value) depending on min/max.
ExprResult CondRes;
if (ReductionOperator == OpenACCReductionOperator::Min)
CondRes = SemaRef.ActOnConditionalOp(Loc, Loc, BinOpRes.get(), LHSDRE,
RHSDRE);
else
CondRes = SemaRef.ActOnConditionalOp(Loc, Loc, BinOpRes.get(), RHSDRE,
LHSDRE);

if (!CondRes.isUsable())
return {CondRes, CombinerFailureKind::Conditional};

// Build assignment.
ExprResult Assignment = SemaRef.BuildBinOp(SemaRef.getCurScope(), Loc,
BinaryOperatorKind::BO_Assign,
LHSDRE, CondRes.get(),
/*ForFoldExpr=*/false);
return {Assignment, Assignment.isUsable()
? CombinerFailureKind::None
: CombinerFailureKind::Assignment};
}
case OpenACCReductionOperator::And:
case OpenACCReductionOperator::Or:
llvm_unreachable("And/Or not implemented, but should fail earlier");
case OpenACCReductionOperator::Invalid:
llvm_unreachable("Invalid should have been caught above");
}
};

auto tryCombiner = [&, this](DeclRefExpr *LHSDRE, DeclRefExpr *RHSDRE,
bool IncludeTrap) {
// TODO: OpenACC: we have to figure out based on the bin-op how to do the
// ones that we can't just use compound operators for. So &&, ||, max, and
// min aren't really clear what we could do here.
if (IncludeTrap) {
// Trap all of the errors here, we'll emit our own at the end.
Sema::TentativeAnalysisScope Trap{SemaRef};

return SemaRef.BuildBinOp(SemaRef.getCurScope(), Loc, BinOp, LHSDRE,
RHSDRE,
/*ForFoldExpr=*/false);
} else {
return SemaRef.BuildBinOp(SemaRef.getCurScope(), Loc, BinOp, LHSDRE,
RHSDRE,
/*ForFoldExpr=*/false);
return genCombiner(LHSDRE, RHSDRE);
}
return genCombiner(LHSDRE, RHSDRE);
};

struct CombinerAttemptTy {
CombinerFailureKind FailKind;
VarDecl *LHS;
DeclRefExpr *LHSDRE;
VarDecl *RHS;
Expand Down Expand Up @@ -3058,9 +3114,11 @@ bool SemaOpenACC::CreateReductionCombinerRecipe(
RHSDecl->getBeginLoc()},
Ty, clang::VK_LValue, RHSDecl, nullptr, NOUR_None);

ExprResult BinOpResult = tryCombiner(LHSDRE, RHSDRE, /*IncludeTrap=*/true);
std::pair<ExprResult, CombinerFailureKind> BinOpResult =
tryCombiner(LHSDRE, RHSDRE, /*IncludeTrap=*/true);

return {LHSDecl, LHSDRE, RHSDecl, RHSDRE, BinOpResult.get()};
return {BinOpResult.second, LHSDecl, LHSDRE, RHSDecl, RHSDRE,
BinOpResult.first.get()};
};

CombinerAttemptTy TopLevelCombinerInfo = formCombiner(VarTy);
Expand All @@ -3081,12 +3139,20 @@ bool SemaOpenACC::CreateReductionCombinerRecipe(
}
}

auto EmitFailureNote = [&](CombinerFailureKind CFK) {
if (CFK == CombinerFailureKind::BinOp)
return Diag(Loc, diag::note_acc_reduction_combiner_forming)
<< CFK << BinaryOperator::getOpcodeStr(BinOp);
return Diag(Loc, diag::note_acc_reduction_combiner_forming) << CFK;
};

// Since the 'root' level didn't fail, the only thing that could be successful
// is a struct that we decompose on its individual fields.

RecordDecl *RD = VarTy->getAsRecordDecl();
if (!RD) {
Diag(Loc, diag::err_acc_reduction_recipe_no_op) << VarTy;
EmitFailureNote(TopLevelCombinerInfo.FailKind);
tryCombiner(TopLevelCombinerInfo.LHSDRE, TopLevelCombinerInfo.RHSDRE,
/*IncludeTrap=*/false);
return true;
Expand All @@ -3098,6 +3164,7 @@ bool SemaOpenACC::CreateReductionCombinerRecipe(
if (!FieldCombinerInfo.Op || FieldCombinerInfo.Op->containsErrors()) {
Diag(Loc, diag::err_acc_reduction_recipe_no_op) << FD->getType();
Diag(FD->getBeginLoc(), diag::note_acc_reduction_recipe_noop_field) << RD;
EmitFailureNote(FieldCombinerInfo.FailKind);
tryCombiner(FieldCombinerInfo.LHSDRE, FieldCombinerInfo.RHSDRE,
/*IncludeTrap=*/false);
return true;
Expand Down
Loading
Loading