[flang][OpenMP] Avoid early returns, NFC #117231
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the first part of the effort to make parsing of clause modifiers more uniform and robust. Currently, when multiple modifiers are allowed, the parser will expect them to appear in a hard-coded order. Additionally, modifier properties (such as "ultimate") are checked separately for each case. The overall plan is 1. Extract all modifiers into their own top-level classes, and then equip them with sets of common properties that will allow performing the property checks generically, without refering to the specific kind of the modifier. 2. Define a parser (as a separate class) for each modifier. 3. For each clause define a union (std::variant) of all allowable modifiers, and parse the modifiers as a list of these unions. The intent is also to isolate parts of the code that could eventually be auto-generated. OpenMP modifier overhaul: #1/3
The main issue to solve is that OpenMP modifiers can be specified in any order, so the parser cannot expect any specific modifier at a given position. To solve that, define modifier to be a union of all allowable specific modifiers for a given clause. Additionally, implement modifier descriptors: for each modifier the corresponding descriptor contains a set of properties of the modifier that allow a common set of semantic checks. Start with the syntactic properties defined in the spec: Required, Unique, Exclusive, Ultimate, and implement common checks to verify each of them. OpenMP modifier overhaul: #2/3
Also, define helper macros in parse-tree.h. Apply the new modifier representation to the DEFAULTMAP and REDUCTION clauses, with testcases utilizing the new modifier validation. OpenMP modifier overhaul: #3/3
…parzysz/spr/m02-openmp-descriptors
…parzysz/spr/m03-semantic-checks
…parzysz/spr/m02-openmp-descriptors
…parzysz/spr/m03-semantic-checks
This actually simplifies the AST node for the schedule clause: the two allowed modifiers can be easily classified as the ordering-modifier and the chunk-modifier during parsing without the need to create additional classes.
Frontend code is generally nested. Follow-up to #116658.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-openmp Author: Krzysztof Parzyszek (kparzysz) ChangesFrontend code is generally nested. Full diff: https://github.com/llvm/llvm-project/pull/117231.diff 2 Files Affected:
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index a4af1ce5771a89..e6398a39d97913 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -2865,45 +2865,41 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Reduction &x) {
bool OmpStructureChecker::CheckReductionOperators(
const parser::OmpClause::Reduction &x) {
+ bool ok = false;
auto &modifiers{OmpGetModifiers(x.v)};
- const auto *definedOp{
- OmpGetUniqueModifier<parser::OmpReductionIdentifier>(modifiers)};
- if (!definedOp) {
- return false;
+ if (const auto *ident{
+ OmpGetUniqueModifier<parser::OmpReductionIdentifier>(modifiers)}) {
+
+ auto visitOperator{[&](const parser::DefinedOperator &dOpr) {
+ if (const auto *intrinsicOp{
+ std::get_if<parser::DefinedOperator::IntrinsicOperator>(
+ &dOpr.u)}) {
+ ok = CheckIntrinsicOperator(*intrinsicOp);
+ } else {
+ context_.Say(GetContext().clauseSource,
+ "Invalid reduction operator in REDUCTION clause."_err_en_US,
+ ContextDirectiveAsFortran());
+ }
+ }};
+
+ auto visitDesignator{[&](const parser::ProcedureDesignator &procD) {
+ const parser::Name *name{std::get_if<parser::Name>(&procD.u)};
+ if (name && name->symbol) {
+ const SourceName &realName{name->symbol->GetUltimate().name()};
+ if (realName == "max" || realName == "min" || realName == "iand" ||
+ realName == "ior" || realName == "ieor") {
+ ok = true;
+ }
+ }
+ if (!ok) {
+ context_.Say(GetContext().clauseSource,
+ "Invalid reduction identifier in REDUCTION "
+ "clause."_err_en_US,
+ ContextDirectiveAsFortran());
+ }
+ }};
+ common::visit(common::visitors{visitOperator, visitDesignator}, ident->u);
}
- bool ok = false;
- common::visit(
- common::visitors{
- [&](const parser::DefinedOperator &dOpr) {
- if (const auto *intrinsicOp{
- std::get_if<parser::DefinedOperator::IntrinsicOperator>(
- &dOpr.u)}) {
- ok = CheckIntrinsicOperator(*intrinsicOp);
- } else {
- context_.Say(GetContext().clauseSource,
- "Invalid reduction operator in REDUCTION clause."_err_en_US,
- ContextDirectiveAsFortran());
- }
- },
- [&](const parser::ProcedureDesignator &procD) {
- const parser::Name *name{std::get_if<parser::Name>(&procD.u)};
- if (name && name->symbol) {
- const SourceName &realName{name->symbol->GetUltimate().name()};
- if (realName == "max" || realName == "min" ||
- realName == "iand" || realName == "ior" ||
- realName == "ieor") {
- ok = true;
- }
- }
- if (!ok) {
- context_.Say(GetContext().clauseSource,
- "Invalid reduction identifier in REDUCTION "
- "clause."_err_en_US,
- ContextDirectiveAsFortran());
- }
- },
- },
- definedOp->u);
return ok;
}
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index c75808a8963b3f..107bd3b09019a0 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -522,49 +522,47 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
const auto &objList{std::get<parser::OmpObjectList>(x.v.t)};
ResolveOmpObjectList(objList, Symbol::Flag::OmpReduction);
- auto &modifiers{OmpGetModifiers(x.v)};
- if (!modifiers) {
- return false;
- }
-
- auto createDummyProcSymbol = [&](const parser::Name *name) {
- // If name resolution failed, create a dummy symbol
- const auto namePair{
- currScope().try_emplace(name->source, Attrs{}, ProcEntityDetails{})};
- auto &newSymbol{*namePair.first->second};
- if (context_.intrinsics().IsIntrinsic(name->ToString())) {
- newSymbol.attrs().set(Attr::INTRINSIC);
- }
- name->symbol = &newSymbol;
- };
+ if (auto &modifiers{OmpGetModifiers(x.v)}) {
+ auto createDummyProcSymbol = [&](const parser::Name *name) {
+ // If name resolution failed, create a dummy symbol
+ const auto namePair{currScope().try_emplace(
+ name->source, Attrs{}, ProcEntityDetails{})};
+ auto &newSymbol{*namePair.first->second};
+ if (context_.intrinsics().IsIntrinsic(name->ToString())) {
+ newSymbol.attrs().set(Attr::INTRINSIC);
+ }
+ name->symbol = &newSymbol;
+ };
- for (auto &mod : *modifiers) {
- if (!std::holds_alternative<parser::OmpReductionIdentifier>(mod.u)) {
- continue;
- }
- auto &opr{std::get<parser::OmpReductionIdentifier>(mod.u)};
- if (auto *procD{parser::Unwrap<parser::ProcedureDesignator>(opr.u)}) {
- if (auto *name{parser::Unwrap<parser::Name>(procD->u)}) {
- if (!name->symbol) {
- if (!ResolveName(name)) {
- createDummyProcSymbol(name);
+ for (auto &mod : *modifiers) {
+ if (!std::holds_alternative<parser::OmpReductionIdentifier>(mod.u)) {
+ continue;
+ }
+ auto &opr{std::get<parser::OmpReductionIdentifier>(mod.u)};
+ if (auto *procD{parser::Unwrap<parser::ProcedureDesignator>(opr.u)}) {
+ if (auto *name{parser::Unwrap<parser::Name>(procD->u)}) {
+ if (!name->symbol) {
+ if (!ResolveName(name)) {
+ createDummyProcSymbol(name);
+ }
}
}
- }
- if (auto *procRef{parser::Unwrap<parser::ProcComponentRef>(procD->u)}) {
- if (!procRef->v.thing.component.symbol) {
- if (!ResolveName(&procRef->v.thing.component)) {
- createDummyProcSymbol(&procRef->v.thing.component);
+ if (auto *procRef{
+ parser::Unwrap<parser::ProcComponentRef>(procD->u)}) {
+ if (!procRef->v.thing.component.symbol) {
+ if (!ResolveName(&procRef->v.thing.component)) {
+ createDummyProcSymbol(&procRef->v.thing.component);
+ }
}
}
}
}
- }
- using ReductionModifier = parser::OmpReductionModifier;
- if (auto *maybeModifier{
- OmpGetUniqueModifier<ReductionModifier>(modifiers)}) {
- if (maybeModifier->v == ReductionModifier::Value::Inscan) {
- ResolveOmpObjectList(objList, Symbol::Flag::OmpInScanReduction);
+ using ReductionModifier = parser::OmpReductionModifier;
+ if (auto *maybeModifier{
+ OmpGetUniqueModifier<ReductionModifier>(modifiers)}) {
+ if (maybeModifier->v == ReductionModifier::Value::Inscan) {
+ ResolveOmpObjectList(objList, Symbol::Flag::OmpInScanReduction);
+ }
}
}
return false;
|
|
@llvm/pr-subscribers-flang-semantics Author: Krzysztof Parzyszek (kparzysz) ChangesFrontend code is generally nested. Full diff: https://github.com/llvm/llvm-project/pull/117231.diff 2 Files Affected:
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index a4af1ce5771a89..e6398a39d97913 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -2865,45 +2865,41 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Reduction &x) {
bool OmpStructureChecker::CheckReductionOperators(
const parser::OmpClause::Reduction &x) {
+ bool ok = false;
auto &modifiers{OmpGetModifiers(x.v)};
- const auto *definedOp{
- OmpGetUniqueModifier<parser::OmpReductionIdentifier>(modifiers)};
- if (!definedOp) {
- return false;
+ if (const auto *ident{
+ OmpGetUniqueModifier<parser::OmpReductionIdentifier>(modifiers)}) {
+
+ auto visitOperator{[&](const parser::DefinedOperator &dOpr) {
+ if (const auto *intrinsicOp{
+ std::get_if<parser::DefinedOperator::IntrinsicOperator>(
+ &dOpr.u)}) {
+ ok = CheckIntrinsicOperator(*intrinsicOp);
+ } else {
+ context_.Say(GetContext().clauseSource,
+ "Invalid reduction operator in REDUCTION clause."_err_en_US,
+ ContextDirectiveAsFortran());
+ }
+ }};
+
+ auto visitDesignator{[&](const parser::ProcedureDesignator &procD) {
+ const parser::Name *name{std::get_if<parser::Name>(&procD.u)};
+ if (name && name->symbol) {
+ const SourceName &realName{name->symbol->GetUltimate().name()};
+ if (realName == "max" || realName == "min" || realName == "iand" ||
+ realName == "ior" || realName == "ieor") {
+ ok = true;
+ }
+ }
+ if (!ok) {
+ context_.Say(GetContext().clauseSource,
+ "Invalid reduction identifier in REDUCTION "
+ "clause."_err_en_US,
+ ContextDirectiveAsFortran());
+ }
+ }};
+ common::visit(common::visitors{visitOperator, visitDesignator}, ident->u);
}
- bool ok = false;
- common::visit(
- common::visitors{
- [&](const parser::DefinedOperator &dOpr) {
- if (const auto *intrinsicOp{
- std::get_if<parser::DefinedOperator::IntrinsicOperator>(
- &dOpr.u)}) {
- ok = CheckIntrinsicOperator(*intrinsicOp);
- } else {
- context_.Say(GetContext().clauseSource,
- "Invalid reduction operator in REDUCTION clause."_err_en_US,
- ContextDirectiveAsFortran());
- }
- },
- [&](const parser::ProcedureDesignator &procD) {
- const parser::Name *name{std::get_if<parser::Name>(&procD.u)};
- if (name && name->symbol) {
- const SourceName &realName{name->symbol->GetUltimate().name()};
- if (realName == "max" || realName == "min" ||
- realName == "iand" || realName == "ior" ||
- realName == "ieor") {
- ok = true;
- }
- }
- if (!ok) {
- context_.Say(GetContext().clauseSource,
- "Invalid reduction identifier in REDUCTION "
- "clause."_err_en_US,
- ContextDirectiveAsFortran());
- }
- },
- },
- definedOp->u);
return ok;
}
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index c75808a8963b3f..107bd3b09019a0 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -522,49 +522,47 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
const auto &objList{std::get<parser::OmpObjectList>(x.v.t)};
ResolveOmpObjectList(objList, Symbol::Flag::OmpReduction);
- auto &modifiers{OmpGetModifiers(x.v)};
- if (!modifiers) {
- return false;
- }
-
- auto createDummyProcSymbol = [&](const parser::Name *name) {
- // If name resolution failed, create a dummy symbol
- const auto namePair{
- currScope().try_emplace(name->source, Attrs{}, ProcEntityDetails{})};
- auto &newSymbol{*namePair.first->second};
- if (context_.intrinsics().IsIntrinsic(name->ToString())) {
- newSymbol.attrs().set(Attr::INTRINSIC);
- }
- name->symbol = &newSymbol;
- };
+ if (auto &modifiers{OmpGetModifiers(x.v)}) {
+ auto createDummyProcSymbol = [&](const parser::Name *name) {
+ // If name resolution failed, create a dummy symbol
+ const auto namePair{currScope().try_emplace(
+ name->source, Attrs{}, ProcEntityDetails{})};
+ auto &newSymbol{*namePair.first->second};
+ if (context_.intrinsics().IsIntrinsic(name->ToString())) {
+ newSymbol.attrs().set(Attr::INTRINSIC);
+ }
+ name->symbol = &newSymbol;
+ };
- for (auto &mod : *modifiers) {
- if (!std::holds_alternative<parser::OmpReductionIdentifier>(mod.u)) {
- continue;
- }
- auto &opr{std::get<parser::OmpReductionIdentifier>(mod.u)};
- if (auto *procD{parser::Unwrap<parser::ProcedureDesignator>(opr.u)}) {
- if (auto *name{parser::Unwrap<parser::Name>(procD->u)}) {
- if (!name->symbol) {
- if (!ResolveName(name)) {
- createDummyProcSymbol(name);
+ for (auto &mod : *modifiers) {
+ if (!std::holds_alternative<parser::OmpReductionIdentifier>(mod.u)) {
+ continue;
+ }
+ auto &opr{std::get<parser::OmpReductionIdentifier>(mod.u)};
+ if (auto *procD{parser::Unwrap<parser::ProcedureDesignator>(opr.u)}) {
+ if (auto *name{parser::Unwrap<parser::Name>(procD->u)}) {
+ if (!name->symbol) {
+ if (!ResolveName(name)) {
+ createDummyProcSymbol(name);
+ }
}
}
- }
- if (auto *procRef{parser::Unwrap<parser::ProcComponentRef>(procD->u)}) {
- if (!procRef->v.thing.component.symbol) {
- if (!ResolveName(&procRef->v.thing.component)) {
- createDummyProcSymbol(&procRef->v.thing.component);
+ if (auto *procRef{
+ parser::Unwrap<parser::ProcComponentRef>(procD->u)}) {
+ if (!procRef->v.thing.component.symbol) {
+ if (!ResolveName(&procRef->v.thing.component)) {
+ createDummyProcSymbol(&procRef->v.thing.component);
+ }
}
}
}
}
- }
- using ReductionModifier = parser::OmpReductionModifier;
- if (auto *maybeModifier{
- OmpGetUniqueModifier<ReductionModifier>(modifiers)}) {
- if (maybeModifier->v == ReductionModifier::Value::Inscan) {
- ResolveOmpObjectList(objList, Symbol::Flag::OmpInScanReduction);
+ using ReductionModifier = parser::OmpReductionModifier;
+ if (auto *maybeModifier{
+ OmpGetUniqueModifier<ReductionModifier>(modifiers)}) {
+ if (maybeModifier->v == ReductionModifier::Value::Inscan) {
+ ResolveOmpObjectList(objList, Symbol::Flag::OmpInScanReduction);
+ }
}
}
return false;
|
Base automatically changed from
users/kparzysz/spr/m04-order-schedule
to
main
November 21, 2024 23:50
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/13427 Here is the relevant piece of the build log for the reference |
|
I'm having trouble building with this change. Same as llvm-ci above. GCC 9.3. |
|
FWIW, I'm getting the same error messages from clang-16.0.6. |
kazutakahirata
added a commit
that referenced
this pull request
Nov 22, 2024
This reverts commit 1a08b15. Buildbot failure: https://lab.llvm.org/buildbot/#/builders/157/builds/13427
|
I've revered the patch. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/15451 Here is the relevant piece of the build log for the reference |
|
There seemed to be a merge/rebase error, where a duplicate definition of a template function was added instead of replacing the previous one. Sorry, I didn't notice it. |
kparzysz
added a commit
that referenced
this pull request
Nov 23, 2024
Two PRs were merged at the same time: one that modified `maybeApplyToV` function, and shortly afterwards, this (the reverted) one that had the old definition. During the merge both definitions were retained leading to compilation errors. Reapply the reverted PR (1a08b15) with the duplicate removed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Frontend code is generally nested.
Follow-up to #116658.