[flang-commits] [flang] [flang][OpenMP] Split check-omp-structure.cpp into smaller files, NFC (PR #146359)
via flang-commits
flang-commits at lists.llvm.org
Mon Jun 30 07:33:00 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-flang-semantics
Author: Krzysztof Parzyszek (kparzysz)
<details>
<summary>Changes</summary>
Create these new files in flang/lib/Semantics:
openmp-utils.cpp/.h - Common utilities
check-omp-atomic.cpp - Atomic-related checks
check-omp-loop.cpp - Loop constructs/clauses
check-omp-metadirective.cpp - Metadirective-related checks
Update lists of included headers, std in particular.
---
Patch is 218.13 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/146359.diff
8 Files Affected:
- (modified) flang/lib/Semantics/CMakeLists.txt (+6-2)
- (added) flang/lib/Semantics/check-omp-atomic.cpp (+1291)
- (added) flang/lib/Semantics/check-omp-loop.cpp (+655)
- (added) flang/lib/Semantics/check-omp-metadirective.cpp (+544)
- (modified) flang/lib/Semantics/check-omp-structure.cpp (+65-2753)
- (modified) flang/lib/Semantics/check-omp-structure.h (-8)
- (added) flang/lib/Semantics/openmp-utils.cpp (+389)
- (added) flang/lib/Semantics/openmp-utils.h (+62)
``````````diff
diff --git a/flang/lib/Semantics/CMakeLists.txt b/flang/lib/Semantics/CMakeLists.txt
index c0fda3631c01f..109bc2dbb8569 100644
--- a/flang/lib/Semantics/CMakeLists.txt
+++ b/flang/lib/Semantics/CMakeLists.txt
@@ -20,6 +20,9 @@ add_flang_library(FortranSemantics
check-io.cpp
check-namelist.cpp
check-nullify.cpp
+ check-omp-atomic.cpp
+ check-omp-loop.cpp
+ check-omp-metadirective.cpp
check-omp-structure.cpp
check-purity.cpp
check-return.cpp
@@ -34,12 +37,13 @@ add_flang_library(FortranSemantics
mod-file.cpp
openmp-dsa.cpp
openmp-modifiers.cpp
+ openmp-utils.cpp
pointer-assignment.cpp
program-tree.cpp
- resolve-labels.cpp
resolve-directives.cpp
- resolve-names-utils.cpp
+ resolve-labels.cpp
resolve-names.cpp
+ resolve-names-utils.cpp
rewrite-parse-tree.cpp
runtime-type-info.cpp
scope.cpp
diff --git a/flang/lib/Semantics/check-omp-atomic.cpp b/flang/lib/Semantics/check-omp-atomic.cpp
new file mode 100644
index 0000000000000..19425a69523d3
--- /dev/null
+++ b/flang/lib/Semantics/check-omp-atomic.cpp
@@ -0,0 +1,1291 @@
+//===-- lib/Semantics/check-omp-atomic.cpp --------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "check-omp-structure.h"
+#include "openmp-utils.h"
+
+#include "flang/Common/indirection.h"
+#include "flang/Evaluate/expression.h"
+#include "flang/Evaluate/tools.h"
+#include "flang/Parser/char-block.h"
+#include "flang/Parser/parse-tree.h"
+#include "flang/Semantics/symbol.h"
+#include "flang/Semantics/tools.h"
+#include "flang/Semantics/type.h"
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Frontend/OpenMP/OMP.h"
+#include "llvm/Support/ErrorHandling.h"
+
+#include <cassert>
+#include <list>
+#include <optional>
+#include <string_view>
+#include <tuple>
+#include <utility>
+#include <variant>
+#include <vector>
+
+namespace Fortran::semantics {
+
+using namespace Fortran::semantics::omp;
+
+namespace operation = Fortran::evaluate::operation;
+
+template <typename T, typename U>
+static bool operator!=(const evaluate::Expr<T> &e, const evaluate::Expr<U> &f) {
+ return !(e == f);
+}
+
+// There is no consistent way to get the source of a given ActionStmt, so
+// extract the source information from Statement<ActionStmt> when we can,
+// and keep it around for error reporting in further analyses.
+struct SourcedActionStmt {
+ const parser::ActionStmt *stmt{nullptr};
+ parser::CharBlock source;
+
+ operator bool() const { return stmt != nullptr; }
+};
+
+struct AnalyzedCondStmt {
+ SomeExpr cond{evaluate::NullPointer{}}; // Default ctor is deleted
+ parser::CharBlock source;
+ SourcedActionStmt ift, iff;
+};
+
+static SourcedActionStmt GetActionStmt(
+ const parser::ExecutionPartConstruct *x) {
+ if (x == nullptr) {
+ return SourcedActionStmt{};
+ }
+ if (auto *exec{std::get_if<parser::ExecutableConstruct>(&x->u)}) {
+ using ActionStmt = parser::Statement<parser::ActionStmt>;
+ if (auto *stmt{std::get_if<ActionStmt>(&exec->u)}) {
+ return SourcedActionStmt{&stmt->statement, stmt->source};
+ }
+ }
+ return SourcedActionStmt{};
+}
+
+static SourcedActionStmt GetActionStmt(const parser::Block &block) {
+ if (block.size() == 1) {
+ return GetActionStmt(&block.front());
+ }
+ return SourcedActionStmt{};
+}
+
+// Compute the `evaluate::Assignment` from parser::ActionStmt. The assumption
+// is that the ActionStmt will be either an assignment or a pointer-assignment,
+// otherwise return std::nullopt.
+// Note: This function can return std::nullopt on [Pointer]AssignmentStmt where
+// the "typedAssignment" is unset. This can happen if there are semantic errors
+// in the purported assignment.
+static std::optional<evaluate::Assignment> GetEvaluateAssignment(
+ const parser::ActionStmt *x) {
+ if (x == nullptr) {
+ return std::nullopt;
+ }
+
+ using AssignmentStmt = common::Indirection<parser::AssignmentStmt>;
+ using PointerAssignmentStmt =
+ common::Indirection<parser::PointerAssignmentStmt>;
+ using TypedAssignment = parser::AssignmentStmt::TypedAssignment;
+
+ return common::visit(
+ [](auto &&s) -> std::optional<evaluate::Assignment> {
+ using BareS = llvm::remove_cvref_t<decltype(s)>;
+ if constexpr (std::is_same_v<BareS, AssignmentStmt> ||
+ std::is_same_v<BareS, PointerAssignmentStmt>) {
+ const TypedAssignment &typed{s.value().typedAssignment};
+ // ForwardOwningPointer typedAssignment
+ // `- GenericAssignmentWrapper ^.get()
+ // `- std::optional<Assignment> ^->v
+ return typed.get()->v;
+ } else {
+ return std::nullopt;
+ }
+ },
+ x->u);
+}
+
+static std::optional<AnalyzedCondStmt> AnalyzeConditionalStmt(
+ const parser::ExecutionPartConstruct *x) {
+ if (x == nullptr) {
+ return std::nullopt;
+ }
+
+ // Extract the evaluate::Expr from ScalarLogicalExpr.
+ auto getFromLogical{[](const parser::ScalarLogicalExpr &logical) {
+ // ScalarLogicalExpr is Scalar<Logical<common::Indirection<Expr>>>
+ const parser::Expr &expr{logical.thing.thing.value()};
+ return GetEvaluateExpr(expr);
+ }};
+
+ // Recognize either
+ // ExecutionPartConstruct -> ExecutableConstruct -> ActionStmt -> IfStmt, or
+ // ExecutionPartConstruct -> ExecutableConstruct -> IfConstruct.
+
+ if (auto &&action{GetActionStmt(x)}) {
+ if (auto *ifs{std::get_if<common::Indirection<parser::IfStmt>>(
+ &action.stmt->u)}) {
+ const parser::IfStmt &s{ifs->value()};
+ auto &&maybeCond{
+ getFromLogical(std::get<parser::ScalarLogicalExpr>(s.t))};
+ auto &thenStmt{
+ std::get<parser::UnlabeledStatement<parser::ActionStmt>>(s.t)};
+ if (maybeCond) {
+ return AnalyzedCondStmt{std::move(*maybeCond), action.source,
+ SourcedActionStmt{&thenStmt.statement, thenStmt.source},
+ SourcedActionStmt{}};
+ }
+ }
+ return std::nullopt;
+ }
+
+ if (auto *exec{std::get_if<parser::ExecutableConstruct>(&x->u)}) {
+ if (auto *ifc{
+ std::get_if<common::Indirection<parser::IfConstruct>>(&exec->u)}) {
+ using ElseBlock = parser::IfConstruct::ElseBlock;
+ using ElseIfBlock = parser::IfConstruct::ElseIfBlock;
+ const parser::IfConstruct &s{ifc->value()};
+
+ if (!std::get<std::list<ElseIfBlock>>(s.t).empty()) {
+ // Not expecting any else-if statements.
+ return std::nullopt;
+ }
+ auto &stmt{std::get<parser::Statement<parser::IfThenStmt>>(s.t)};
+ auto &&maybeCond{getFromLogical(
+ std::get<parser::ScalarLogicalExpr>(stmt.statement.t))};
+ if (!maybeCond) {
+ return std::nullopt;
+ }
+
+ if (auto &maybeElse{std::get<std::optional<ElseBlock>>(s.t)}) {
+ AnalyzedCondStmt result{std::move(*maybeCond), stmt.source,
+ GetActionStmt(std::get<parser::Block>(s.t)),
+ GetActionStmt(std::get<parser::Block>(maybeElse->t))};
+ if (result.ift.stmt && result.iff.stmt) {
+ return result;
+ }
+ } else {
+ AnalyzedCondStmt result{std::move(*maybeCond), stmt.source,
+ GetActionStmt(std::get<parser::Block>(s.t)), SourcedActionStmt{}};
+ if (result.ift.stmt) {
+ return result;
+ }
+ }
+ }
+ return std::nullopt;
+ }
+
+ return std::nullopt;
+}
+
+static std::pair<parser::CharBlock, parser::CharBlock> SplitAssignmentSource(
+ parser::CharBlock source) {
+ // Find => in the range, if not found, find = that is not a part of
+ // <=, >=, ==, or /=.
+ auto trim{[](std::string_view v) {
+ const char *begin{v.data()};
+ const char *end{begin + v.size()};
+ while (*begin == ' ' && begin != end) {
+ ++begin;
+ }
+ while (begin != end && end[-1] == ' ') {
+ --end;
+ }
+ assert(begin != end && "Source should not be empty");
+ return parser::CharBlock(begin, end - begin);
+ }};
+
+ std::string_view sv(source.begin(), source.size());
+
+ if (auto where{sv.find("=>")}; where != sv.npos) {
+ std::string_view lhs(sv.data(), where);
+ std::string_view rhs(sv.data() + where + 2, sv.size() - where - 2);
+ return std::make_pair(trim(lhs), trim(rhs));
+ }
+
+ // Go backwards, since all the exclusions above end with a '='.
+ for (size_t next{source.size()}; next > 1; --next) {
+ if (sv[next - 1] == '=' && !llvm::is_contained("<>=/", sv[next - 2])) {
+ std::string_view lhs(sv.data(), next - 1);
+ std::string_view rhs(sv.data() + next, sv.size() - next);
+ return std::make_pair(trim(lhs), trim(rhs));
+ }
+ }
+ llvm_unreachable("Could not find assignment operator");
+}
+
+static bool IsCheckForAssociated(const SomeExpr &cond) {
+ return GetTopLevelOperation(cond).first == operation::Operator::Associated;
+}
+
+static bool IsMaybeAtomicWrite(const evaluate::Assignment &assign) {
+ // This ignores function calls, so it will accept "f(x) = f(x) + 1"
+ // for example.
+ return HasStorageOverlap(assign.lhs, assign.rhs) == nullptr;
+}
+
+static void SetExpr(parser::TypedExpr &expr, MaybeExpr value) {
+ if (value) {
+ expr.Reset(new evaluate::GenericExprWrapper(std::move(value)),
+ evaluate::GenericExprWrapper::Deleter);
+ }
+}
+
+static void SetAssignment(parser::AssignmentStmt::TypedAssignment &assign,
+ std::optional<evaluate::Assignment> value) {
+ if (value) {
+ assign.Reset(new evaluate::GenericAssignmentWrapper(std::move(value)),
+ evaluate::GenericAssignmentWrapper::Deleter);
+ }
+}
+
+static parser::OpenMPAtomicConstruct::Analysis::Op MakeAtomicAnalysisOp(
+ int what,
+ const std::optional<evaluate::Assignment> &maybeAssign = std::nullopt) {
+ parser::OpenMPAtomicConstruct::Analysis::Op operation;
+ operation.what = what;
+ SetAssignment(operation.assign, maybeAssign);
+ return operation;
+}
+
+static parser::OpenMPAtomicConstruct::Analysis MakeAtomicAnalysis(
+ const SomeExpr &atom, const MaybeExpr &cond,
+ parser::OpenMPAtomicConstruct::Analysis::Op &&op0,
+ parser::OpenMPAtomicConstruct::Analysis::Op &&op1) {
+ // Defined in flang/include/flang/Parser/parse-tree.h
+ //
+ // struct Analysis {
+ // struct Kind {
+ // static constexpr int None = 0;
+ // static constexpr int Read = 1;
+ // static constexpr int Write = 2;
+ // static constexpr int Update = Read | Write;
+ // static constexpr int Action = 3; // Bits containing N, R, W, U
+ // static constexpr int IfTrue = 4;
+ // static constexpr int IfFalse = 8;
+ // static constexpr int Condition = 12; // Bits containing IfTrue, IfFalse
+ // };
+ // struct Op {
+ // int what;
+ // TypedAssignment assign;
+ // };
+ // TypedExpr atom, cond;
+ // Op op0, op1;
+ // };
+
+ parser::OpenMPAtomicConstruct::Analysis an;
+ SetExpr(an.atom, atom);
+ SetExpr(an.cond, cond);
+ an.op0 = std::move(op0);
+ an.op1 = std::move(op1);
+ return an;
+}
+
+/// Check if `expr` satisfies the following conditions for x and v:
+///
+/// [6.0:189:10-12]
+/// - x and v (as applicable) are either scalar variables or
+/// function references with scalar data pointer result of non-character
+/// intrinsic type or variables that are non-polymorphic scalar pointers
+/// and any length type parameter must be constant.
+void OmpStructureChecker::CheckAtomicType(
+ SymbolRef sym, parser::CharBlock source, std::string_view name) {
+ const DeclTypeSpec *typeSpec{sym->GetType()};
+ if (!typeSpec) {
+ return;
+ }
+
+ if (!IsPointer(sym)) {
+ using Category = DeclTypeSpec::Category;
+ Category cat{typeSpec->category()};
+ if (cat == Category::Character) {
+ context_.Say(source,
+ "Atomic variable %s cannot have CHARACTER type"_err_en_US, name);
+ } else if (cat != Category::Numeric && cat != Category::Logical) {
+ context_.Say(source,
+ "Atomic variable %s should have an intrinsic type"_err_en_US, name);
+ }
+ return;
+ }
+
+ // Variable is a pointer.
+ if (typeSpec->IsPolymorphic()) {
+ context_.Say(source,
+ "Atomic variable %s cannot be a pointer to a polymorphic type"_err_en_US,
+ name);
+ return;
+ }
+
+ // Go over all length parameters, if any, and check if they are
+ // explicit.
+ if (const DerivedTypeSpec *derived{typeSpec->AsDerived()}) {
+ if (llvm::any_of(derived->parameters(), [](auto &&entry) {
+ // "entry" is a map entry
+ return entry.second.isLen() && !entry.second.isExplicit();
+ })) {
+ context_.Say(source,
+ "Atomic variable %s is a pointer to a type with non-constant length parameter"_err_en_US,
+ name);
+ }
+ }
+}
+
+void OmpStructureChecker::CheckAtomicVariable(
+ const SomeExpr &atom, parser::CharBlock source) {
+ if (atom.Rank() != 0) {
+ context_.Say(source, "Atomic variable %s should be a scalar"_err_en_US,
+ atom.AsFortran());
+ }
+
+ std::vector<SomeExpr> dsgs{GetAllDesignators(atom)};
+ assert(dsgs.size() == 1 && "Should have a single top-level designator");
+ evaluate::SymbolVector syms{evaluate::GetSymbolVector(dsgs.front())};
+
+ CheckAtomicType(syms.back(), source, atom.AsFortran());
+
+ if (IsAllocatable(syms.back()) && !IsArrayElement(atom)) {
+ context_.Say(source, "Atomic variable %s cannot be ALLOCATABLE"_err_en_US,
+ atom.AsFortran());
+ }
+}
+
+void OmpStructureChecker::CheckStorageOverlap(const SomeExpr &base,
+ llvm::ArrayRef<evaluate::Expr<evaluate::SomeType>> exprs,
+ parser::CharBlock source) {
+ if (auto *expr{HasStorageOverlap(base, exprs)}) {
+ context_.Say(source,
+ "Within atomic operation %s and %s access the same storage"_warn_en_US,
+ base.AsFortran(), expr->AsFortran());
+ }
+}
+
+void OmpStructureChecker::ErrorShouldBeVariable(
+ const MaybeExpr &expr, parser::CharBlock source) {
+ if (expr) {
+ context_.Say(source, "Atomic expression %s should be a variable"_err_en_US,
+ expr->AsFortran());
+ } else {
+ context_.Say(source, "Atomic expression should be a variable"_err_en_US);
+ }
+}
+
+std::pair<const parser::ExecutionPartConstruct *,
+ const parser::ExecutionPartConstruct *>
+OmpStructureChecker::CheckUpdateCapture(
+ const parser::ExecutionPartConstruct *ec1,
+ const parser::ExecutionPartConstruct *ec2, parser::CharBlock source) {
+ // Decide which statement is the atomic update and which is the capture.
+ //
+ // The two allowed cases are:
+ // x = ... atomic-var = ...
+ // ... = x capture-var = atomic-var (with optional converts)
+ // or
+ // ... = x capture-var = atomic-var (with optional converts)
+ // x = ... atomic-var = ...
+ //
+ // The case of 'a = b; b = a' is ambiguous, so pick the first one as capture
+ // (which makes more sense, as it captures the original value of the atomic
+ // variable).
+ //
+ // If the two statements don't fit these criteria, return a pair of default-
+ // constructed values.
+ using ReturnTy = std::pair<const parser::ExecutionPartConstruct *,
+ const parser::ExecutionPartConstruct *>;
+
+ SourcedActionStmt act1{GetActionStmt(ec1)};
+ SourcedActionStmt act2{GetActionStmt(ec2)};
+ auto maybeAssign1{GetEvaluateAssignment(act1.stmt)};
+ auto maybeAssign2{GetEvaluateAssignment(act2.stmt)};
+ if (!maybeAssign1 || !maybeAssign2) {
+ if (!IsAssignment(act1.stmt) || !IsAssignment(act2.stmt)) {
+ context_.Say(source,
+ "ATOMIC UPDATE operation with CAPTURE should contain two assignments"_err_en_US);
+ }
+ return std::make_pair(nullptr, nullptr);
+ }
+
+ auto as1{*maybeAssign1}, as2{*maybeAssign2};
+
+ auto isUpdateCapture{
+ [](const evaluate::Assignment &u, const evaluate::Assignment &c) {
+ return IsSameOrConvertOf(c.rhs, u.lhs);
+ }};
+
+ // Do some checks that narrow down the possible choices for the update
+ // and the capture statements. This will help to emit better diagnostics.
+ // 1. An assignment could be an update (cbu) if the left-hand side is a
+ // subexpression of the right-hand side.
+ // 2. An assignment could be a capture (cbc) if the right-hand side is
+ // a variable (or a function ref), with potential type conversions.
+ bool cbu1{IsSubexpressionOf(as1.lhs, as1.rhs)}; // Can as1 be an update?
+ bool cbu2{IsSubexpressionOf(as2.lhs, as2.rhs)}; // Can as2 be an update?
+ bool cbc1{IsVarOrFunctionRef(GetConvertInput(as1.rhs))}; // Can 1 be capture?
+ bool cbc2{IsVarOrFunctionRef(GetConvertInput(as2.rhs))}; // Can 2 be capture?
+
+ // We want to diagnose cases where both assignments cannot be an update,
+ // or both cannot be a capture, as well as cases where either assignment
+ // cannot be any of these two.
+ //
+ // If we organize these boolean values into a matrix
+ // |cbu1 cbu2|
+ // |cbc1 cbc2|
+ // then we want to diagnose cases where the matrix has a zero (i.e. "false")
+ // row or column, including the case where everything is zero. All these
+ // cases correspond to the determinant of the matrix being 0, which suggests
+ // that checking the det may be a convenient diagnostic check. There is only
+ // one additional case where the det is 0, which is when the matrix is all 1
+ // ("true"). The "all true" case represents the situation where both
+ // assignments could be an update as well as a capture. On the other hand,
+ // whenever det != 0, the roles of the update and the capture can be
+ // unambiguously assigned to as1 and as2 [1].
+ //
+ // [1] This can be easily verified by hand: there are 10 2x2 matrices with
+ // det = 0, leaving 6 cases where det != 0:
+ // 0 1 0 1 1 0 1 0 1 1 1 1
+ // 1 0 1 1 0 1 1 1 0 1 1 0
+ // In each case the classification is unambiguous.
+
+ // |cbu1 cbu2|
+ // det |cbc1 cbc2| = cbu1*cbc2 - cbu2*cbc1
+ int det{int(cbu1) * int(cbc2) - int(cbu2) * int(cbc1)};
+
+ auto errorCaptureShouldRead{[&](const parser::CharBlock &source,
+ const std::string &expr) {
+ context_.Say(source,
+ "In ATOMIC UPDATE operation with CAPTURE the right-hand side of the capture assignment should read %s"_err_en_US,
+ expr);
+ }};
+
+ auto errorNeitherWorks{[&]() {
+ context_.Say(source,
+ "In ATOMIC UPDATE operation with CAPTURE neither statement could be the update or the capture"_err_en_US);
+ }};
+
+ auto makeSelectionFromDet{[&](int det) -> ReturnTy {
+ // If det != 0, then the checks unambiguously suggest a specific
+ // categorization.
+ // If det == 0, then this function should be called only if the
+ // checks haven't ruled out any possibility, i.e. when both assigments
+ // could still be either updates or captures.
+ if (det > 0) {
+ // as1 is update, as2 is capture
+ if (isUpdateCapture(as1, as2)) {
+ return std::make_pair(/*Update=*/ec1, /*Capture=*/ec2);
+ } else {
+ errorCaptureShouldRead(act2.source, as1.lhs.AsFortran());
+ return std::make_pair(nullptr, nullptr);
+ }
+ } else if (det < 0) {
+ // as2 is update, as1 is capture
+ if (isUpdateCapture(as2, as1)) {
+ return std::make_pair(/*Update=*/ec2, /*Capture=*/ec1);
+ } else {
+ errorCaptureShouldRead(act1.source, as2.lhs.AsFortran());
+ return std::make_pair(nullptr, nullptr);
+ }
+ } else {
+ bool updateFirst{isUpdateCapture(as1, as2)};
+ bool captureFirst{isUpdateCapture(as2, as1)};
+ if (updateFirst && captureFirst) {
+ // If both assignment could be the update and both could be the
+ // capture, emit a warning about the ambiguity.
+ context_.Say(act1.source,
+ "In ATOMIC UPDATE operation with CAPTURE either statement could be the update and the capture, assuming the first one is the capture statement"_warn_en_US);
+ return std::make_pair(/*Update=*/ec2, /*Capture=*/ec1);
+ }
+ if (updateFirst != captureFirst) {
+ const parser::ExecutionPartConstruct *upd{updateFirst ? ec1 : ec2};
+ const parser::ExecutionPartConstruct *cap{captureFirst ? ec1 : ec2};
+ return std::...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/146359
More information about the flang-commits
mailing list