[flang][Evaluate] OperationCode cleanup, fix for Constant<T> #151566
Merged
+31
−39
Conversation
This file contains hidden or 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
Make the OperationCode overloads take the derived operation instead of the Operation base class instance. This makes them usable from visitors of "Expr<T>.u". Also, fix small bug: OperationCode(Constant<T>) shoud be "Constant".
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-openmp Author: Krzysztof Parzyszek (kparzysz) ChangesMake the OperationCode overloads take the derived operation instead of the Operation base class instance. This makes them usable from visitors of "Expr<T>.u". Also, fix small bug: OperationCode(Constant<T>) shoud be "Constant". Full diff: https://github.com/llvm/llvm-project/pull/151566.diff 4 Files Affected:
diff --git a/flang/include/flang/Evaluate/tools.h b/flang/include/flang/Evaluate/tools.h
index cef57f1851bcc..e2c9878ab19a9 100644
--- a/flang/include/flang/Evaluate/tools.h
+++ b/flang/include/flang/Evaluate/tools.h
@@ -1402,10 +1402,8 @@ using OperatorSet = common::EnumSet<Operator, 32>;
std::string ToString(Operator op);
-template <typename... Ts, int Kind>
-Operator OperationCode(
- const evaluate::Operation<evaluate::LogicalOperation<Kind>, Ts...> &op) {
- switch (op.derived().logicalOperator) {
+template <int Kind> Operator OperationCode(const LogicalOperation<Kind> &op) {
+ switch (op.logicalOperator) {
case common::LogicalOperator::And:
return Operator::And;
case common::LogicalOperator::Or:
@@ -1420,10 +1418,8 @@ Operator OperationCode(
return Operator::Unknown;
}
-template <typename T, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::Relational<T>, Ts...> &op) {
- switch (op.derived().opr) {
+template <typename T> Operator OperationCode(const Relational<T> &op) {
+ switch (op.opr) {
case common::RelationalOperator::LT:
return Operator::Lt;
case common::RelationalOperator::LE:
@@ -1440,44 +1436,32 @@ Operator OperationCode(
return Operator::Unknown;
}
-template <typename T, typename... Ts>
-Operator OperationCode(const evaluate::Operation<evaluate::Add<T>, Ts...> &op) {
+template <typename T> Operator OperationCode(const Add<T> &op) {
return Operator::Add;
}
-template <typename T, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::Subtract<T>, Ts...> &op) {
+template <typename T> Operator OperationCode(const Subtract<T> &op) {
return Operator::Sub;
}
-template <typename T, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::Multiply<T>, Ts...> &op) {
+template <typename T> Operator OperationCode(const Multiply<T> &op) {
return Operator::Mul;
}
-template <typename T, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::Divide<T>, Ts...> &op) {
+template <typename T> Operator OperationCode(const Divide<T> &op) {
return Operator::Div;
}
-template <typename T, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::Power<T>, Ts...> &op) {
+template <typename T> Operator OperationCode(const Power<T> &op) {
return Operator::Pow;
}
-template <typename T, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::RealToIntPower<T>, Ts...> &op) {
+template <typename T> Operator OperationCode(const RealToIntPower<T> &op) {
return Operator::Pow;
}
-template <typename T, common::TypeCategory C, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::Convert<T, C>, Ts...> &op) {
+template <typename T, common::TypeCategory C>
+Operator OperationCode(const Convert<T, C> &op) {
if constexpr (C == T::category) {
return Operator::Resize;
} else {
@@ -1485,25 +1469,27 @@ Operator OperationCode(
}
}
-template <typename T, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::Extremum<T>, Ts...> &op) {
- if (op.derived().ordering == evaluate::Ordering::Greater) {
+template <typename T> Operator OperationCode(const Extremum<T> &op) {
+ if (op.ordering == Ordering::Greater) {
return Operator::Max;
} else {
return Operator::Min;
}
}
-template <typename T> Operator OperationCode(const evaluate::Constant<T> &x) {
+template <typename T> Operator OperationCode(const Constant<T> &x) {
return Operator::Constant;
}
+template <typename T> Operator OperationCode(const Designator<T> &x) {
+ return Operator::Identity;
+}
+
template <typename T> Operator OperationCode(const T &) {
return Operator::Unknown;
}
-Operator OperationCode(const evaluate::ProcedureDesignator &proc);
+Operator OperationCode(const ProcedureDesignator &proc);
} // namespace operation
diff --git a/flang/lib/Evaluate/tools.cpp b/flang/lib/Evaluate/tools.cpp
index 171dd91fa9fd1..90be131651697 100644
--- a/flang/lib/Evaluate/tools.cpp
+++ b/flang/lib/Evaluate/tools.cpp
@@ -1693,17 +1693,17 @@ struct ArgumentExtractor
// to int(kind=4) for example.
return (*this)(x.template operand<0>());
} else {
- return std::make_pair(operation::OperationCode(x),
+ return std::make_pair(operation::OperationCode(x.derived()),
OperationArgs(x, std::index_sequence_for<Os...>{}));
}
}
template <typename T> Result operator()(const Designator<T> &x) const {
- return {operation::Operator::Identity, {AsSomeExpr(x)}};
+ return {operation::OperationCode(x), {AsSomeExpr(x)}};
}
template <typename T> Result operator()(const Constant<T> &x) const {
- return {operation::Operator::Identity, {AsSomeExpr(x)}};
+ return {operation::OperationCode(x), {AsSomeExpr(x)}};
}
template <typename... Rs>
diff --git a/flang/test/Semantics/OpenMP/atomic04.f90 b/flang/test/Semantics/OpenMP/atomic04.f90
index fb87ca5186612..8f8af31245404 100644
--- a/flang/test/Semantics/OpenMP/atomic04.f90
+++ b/flang/test/Semantics/OpenMP/atomic04.f90
@@ -180,7 +180,7 @@ subroutine more_invalid_atomic_update_stmts()
x = x
!$omp atomic update
- !ERROR: The atomic variable x should appear as an argument in the update operation
+ !ERROR: This is not a valid ATOMIC UPDATE operation
x = 1
!$omp atomic update
diff --git a/flang/test/Semantics/OpenMP/atomic05.f90 b/flang/test/Semantics/OpenMP/atomic05.f90
index 77ffc6e57f1a3..e0103be4cae4a 100644
--- a/flang/test/Semantics/OpenMP/atomic05.f90
+++ b/flang/test/Semantics/OpenMP/atomic05.f90
@@ -19,7 +19,7 @@ program OmpAtomic
x = 2 * 4
!ERROR: At most one clause from the 'memory-order' group is allowed on ATOMIC construct
!$omp atomic update release, seq_cst
- !ERROR: The atomic variable x should appear as an argument in the update operation
+ !ERROR: This is not a valid ATOMIC UPDATE operation
x = 10
!ERROR: At most one clause from the 'memory-order' group is allowed on ATOMIC construct
!$omp atomic capture release, seq_cst
|
|
@llvm/pr-subscribers-flang-semantics Author: Krzysztof Parzyszek (kparzysz) ChangesMake the OperationCode overloads take the derived operation instead of the Operation base class instance. This makes them usable from visitors of "Expr<T>.u". Also, fix small bug: OperationCode(Constant<T>) shoud be "Constant". Full diff: https://github.com/llvm/llvm-project/pull/151566.diff 4 Files Affected:
diff --git a/flang/include/flang/Evaluate/tools.h b/flang/include/flang/Evaluate/tools.h
index cef57f1851bcc..e2c9878ab19a9 100644
--- a/flang/include/flang/Evaluate/tools.h
+++ b/flang/include/flang/Evaluate/tools.h
@@ -1402,10 +1402,8 @@ using OperatorSet = common::EnumSet<Operator, 32>;
std::string ToString(Operator op);
-template <typename... Ts, int Kind>
-Operator OperationCode(
- const evaluate::Operation<evaluate::LogicalOperation<Kind>, Ts...> &op) {
- switch (op.derived().logicalOperator) {
+template <int Kind> Operator OperationCode(const LogicalOperation<Kind> &op) {
+ switch (op.logicalOperator) {
case common::LogicalOperator::And:
return Operator::And;
case common::LogicalOperator::Or:
@@ -1420,10 +1418,8 @@ Operator OperationCode(
return Operator::Unknown;
}
-template <typename T, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::Relational<T>, Ts...> &op) {
- switch (op.derived().opr) {
+template <typename T> Operator OperationCode(const Relational<T> &op) {
+ switch (op.opr) {
case common::RelationalOperator::LT:
return Operator::Lt;
case common::RelationalOperator::LE:
@@ -1440,44 +1436,32 @@ Operator OperationCode(
return Operator::Unknown;
}
-template <typename T, typename... Ts>
-Operator OperationCode(const evaluate::Operation<evaluate::Add<T>, Ts...> &op) {
+template <typename T> Operator OperationCode(const Add<T> &op) {
return Operator::Add;
}
-template <typename T, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::Subtract<T>, Ts...> &op) {
+template <typename T> Operator OperationCode(const Subtract<T> &op) {
return Operator::Sub;
}
-template <typename T, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::Multiply<T>, Ts...> &op) {
+template <typename T> Operator OperationCode(const Multiply<T> &op) {
return Operator::Mul;
}
-template <typename T, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::Divide<T>, Ts...> &op) {
+template <typename T> Operator OperationCode(const Divide<T> &op) {
return Operator::Div;
}
-template <typename T, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::Power<T>, Ts...> &op) {
+template <typename T> Operator OperationCode(const Power<T> &op) {
return Operator::Pow;
}
-template <typename T, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::RealToIntPower<T>, Ts...> &op) {
+template <typename T> Operator OperationCode(const RealToIntPower<T> &op) {
return Operator::Pow;
}
-template <typename T, common::TypeCategory C, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::Convert<T, C>, Ts...> &op) {
+template <typename T, common::TypeCategory C>
+Operator OperationCode(const Convert<T, C> &op) {
if constexpr (C == T::category) {
return Operator::Resize;
} else {
@@ -1485,25 +1469,27 @@ Operator OperationCode(
}
}
-template <typename T, typename... Ts>
-Operator OperationCode(
- const evaluate::Operation<evaluate::Extremum<T>, Ts...> &op) {
- if (op.derived().ordering == evaluate::Ordering::Greater) {
+template <typename T> Operator OperationCode(const Extremum<T> &op) {
+ if (op.ordering == Ordering::Greater) {
return Operator::Max;
} else {
return Operator::Min;
}
}
-template <typename T> Operator OperationCode(const evaluate::Constant<T> &x) {
+template <typename T> Operator OperationCode(const Constant<T> &x) {
return Operator::Constant;
}
+template <typename T> Operator OperationCode(const Designator<T> &x) {
+ return Operator::Identity;
+}
+
template <typename T> Operator OperationCode(const T &) {
return Operator::Unknown;
}
-Operator OperationCode(const evaluate::ProcedureDesignator &proc);
+Operator OperationCode(const ProcedureDesignator &proc);
} // namespace operation
diff --git a/flang/lib/Evaluate/tools.cpp b/flang/lib/Evaluate/tools.cpp
index 171dd91fa9fd1..90be131651697 100644
--- a/flang/lib/Evaluate/tools.cpp
+++ b/flang/lib/Evaluate/tools.cpp
@@ -1693,17 +1693,17 @@ struct ArgumentExtractor
// to int(kind=4) for example.
return (*this)(x.template operand<0>());
} else {
- return std::make_pair(operation::OperationCode(x),
+ return std::make_pair(operation::OperationCode(x.derived()),
OperationArgs(x, std::index_sequence_for<Os...>{}));
}
}
template <typename T> Result operator()(const Designator<T> &x) const {
- return {operation::Operator::Identity, {AsSomeExpr(x)}};
+ return {operation::OperationCode(x), {AsSomeExpr(x)}};
}
template <typename T> Result operator()(const Constant<T> &x) const {
- return {operation::Operator::Identity, {AsSomeExpr(x)}};
+ return {operation::OperationCode(x), {AsSomeExpr(x)}};
}
template <typename... Rs>
diff --git a/flang/test/Semantics/OpenMP/atomic04.f90 b/flang/test/Semantics/OpenMP/atomic04.f90
index fb87ca5186612..8f8af31245404 100644
--- a/flang/test/Semantics/OpenMP/atomic04.f90
+++ b/flang/test/Semantics/OpenMP/atomic04.f90
@@ -180,7 +180,7 @@ subroutine more_invalid_atomic_update_stmts()
x = x
!$omp atomic update
- !ERROR: The atomic variable x should appear as an argument in the update operation
+ !ERROR: This is not a valid ATOMIC UPDATE operation
x = 1
!$omp atomic update
diff --git a/flang/test/Semantics/OpenMP/atomic05.f90 b/flang/test/Semantics/OpenMP/atomic05.f90
index 77ffc6e57f1a3..e0103be4cae4a 100644
--- a/flang/test/Semantics/OpenMP/atomic05.f90
+++ b/flang/test/Semantics/OpenMP/atomic05.f90
@@ -19,7 +19,7 @@ program OmpAtomic
x = 2 * 4
!ERROR: At most one clause from the 'memory-order' group is allowed on ATOMIC construct
!$omp atomic update release, seq_cst
- !ERROR: The atomic variable x should appear as an argument in the update operation
+ !ERROR: This is not a valid ATOMIC UPDATE operation
x = 10
!ERROR: At most one clause from the 'memory-order' group is allowed on ATOMIC construct
!$omp atomic capture release, seq_cst
|
tblah
reviewed
Jul 31, 2025
| @@ -180,7 +180,7 @@ subroutine more_invalid_atomic_update_stmts() | |||
| x = x | |||
|
|
|||
| !$omp atomic update | |||
| !ERROR: The atomic variable x should appear as an argument in the update operation | |||
| !ERROR: This is not a valid ATOMIC UPDATE operation | |||
There was a problem hiding this comment.
Is the bugfix the functional change here?
There was a problem hiding this comment.
Yes.
Edit: The problem shows for "x = <constant>". For Constant<T> it used to return "Identity" as the operation code, which is the code for Designator<T>. "Identity" (i.e. x = x) is a valid update operation (as an extension), so we checked for the presence of x among the operands to make sure it's x = x, and not x = y (hence the previous message). "Constant" is not a valid operation, and the new message reflects that.
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.
Make the OperationCode overloads take the derived operation instead of the Operation base class instance. This makes them usable from visitors of "Expr.u".
Also, fix small bug: OperationCode(Constant) shoud be "Constant".