address feedback

Author: akuhlens

flang/include/flang/Evaluate/tools.h

@@ -10,6 +10,7 @@
 #define FORTRAN_EVALUATE_TOOLS_H_
 
 #include "traverse.h"
+#include "flang/Common/enum-set.h"
 #include "flang/Common/idioms.h"
 #include "flang/Common/template.h"
 #include "flang/Common/unwrap.h"
@@ -1397,6 +1398,8 @@ enum class Operator {
   True,
 };
 
+using OperatorSet = common::EnumSet<Operator, 32>;
+
 std::string ToString(Operator op);
 
 template <typename... Ts, int Kind>
@@ -1509,6 +1512,11 @@ Operator OperationCode(const evaluate::ProcedureDesignator &proc);
 std::pair<operation::Operator, std::vector<Expr<SomeType>>>
 GetTopLevelOperation(const Expr<SomeType> &expr);
 
+// Return information about the top-level operation (ignoring parentheses, and
+// resizing converts)
+std::pair<operation::Operator, std::vector<Expr<SomeType>>>
+GetTopLevelOperationIgnoreResizing(const Expr<SomeType> &expr);
+
 // Check if expr is same as x, or a sequence of Convert operations on x.
 bool IsSameOrConvertOf(const Expr<SomeType> &expr, const Expr<SomeType> &x);
 

flang/lib/Evaluate/tools.cpp

@@ -1809,10 +1809,15 @@ operation::Operator operation::OperationCode(const ProcedureDesignator &proc) {
 }
 
 std::pair<operation::Operator, std::vector<Expr<SomeType>>>
-GetTopLevelOperation(const Expr<SomeType> &expr) {
+GetTopLevelOperationIgnoreResizing(const Expr<SomeType> &expr) {
   return operation::ArgumentExtractor<true>{}(expr);
 }
 
+std::pair<operation::Operator, std::vector<Expr<SomeType>>>
+GetTopLevelOperation(const Expr<SomeType> &expr) {
+  return operation::ArgumentExtractor<false>{}(expr);
+}
+
 namespace operation {
 struct ConvertCollector
     : public Traverse<ConvertCollector,
@@ -1946,14 +1951,12 @@ struct VariableFinder : public evaluate::AnyTraverse<VariableFinder> {
 
   template <typename T>
   bool operator()(const evaluate::Designator<T> &x) const {
-    auto copy{x};
-    return evaluate::AsGenericExpr(std::move(copy)) == var;
+    return evaluate::AsGenericExpr(common::Clone(x)) == var;
   }
 
   template <typename T>
   bool operator()(const evaluate::FunctionRef<T> &x) const {
-    auto copy{x};
-    return evaluate::AsGenericExpr(std::move(copy)) == var;
+    return evaluate::AsGenericExpr(common::Clone(x)) == var;
   }
 
 private:

flang/lib/Lower/OpenMP/Atomic.cpp

@@ -607,7 +607,7 @@ genAtomicUpdate(lower::AbstractConverter &converter,
   // This must exist by now.
   semantics::SomeExpr rhs = assign.rhs;
   semantics::SomeExpr input = *evaluate::GetConvertInput(rhs);
-  auto [opcode, args] = evaluate::GetTopLevelOperation(input);
+  auto [opcode, args] = evaluate::GetTopLevelOperationIgnoreResizing(input);
   assert(!args.empty() && "Update operation without arguments");
 
   // Pass args as an argument to avoid capturing a structured binding.
@@ -625,7 +625,8 @@ genAtomicUpdate(lower::AbstractConverter &converter,
     // operations with exactly two (non-optional) arguments.
     rhs = genReducedMinMax(rhs, atomArg, args);
     input = *evaluate::GetConvertInput(rhs);
-    std::tie(opcode, args) = evaluate::GetTopLevelOperation(input);
+    std::tie(opcode, args) =
+        evaluate::GetTopLevelOperationIgnoreResizing(input);
     atomArg = nullptr; // No longer valid.
   }
   for (auto &arg : args) {

flang/lib/Semantics/check-acc-structure.cpp

@@ -14,12 +14,8 @@
 #include "flang/Semantics/type.h"
 #include "flang/Support/Fortran.h"
 #include "llvm/Support/AtomicOrdering.h"
-#include "llvm/Support/raw_ostream.h"
-#include <optional>
 
-// TODO: Move the parts that I am reusing from openmp-utils.h to
-// evaluate/tools.h
-#include "openmp-utils.h"
+#include <optional>
 
 #define CHECK_SIMPLE_CLAUSE(X, Y) \
   void AccStructureChecker::Enter(const parser::AccClause::X &) { \
@@ -354,7 +350,7 @@ void AccStructureChecker::Leave(const parser::OpenACCAtomicConstruct &x) {
 }
 
 void AccStructureChecker::CheckAtomicStmt(
-    const parser::AssignmentStmt &assign, std::string &construct) {
+    const parser::AssignmentStmt &assign, const std::string &construct) {
   const auto &var{std::get<parser::Variable>(assign.t)};
   const auto &expr{std::get<parser::Expr>(assign.t)};
   const auto *rhs{GetExpr(context_, expr)};
@@ -376,38 +372,30 @@ void AccStructureChecker::CheckAtomicStmt(
   }
 }
 
+static constexpr evaluate::operation::OperatorSet validAccAtomicUpdateOperators{
+    evaluate::operation::Operator::Add, evaluate::operation::Operator::Mul,
+    evaluate::operation::Operator::Sub, evaluate::operation::Operator::Div,
+    evaluate::operation::Operator::And, evaluate::operation::Operator::Or,
+    evaluate::operation::Operator::Eqv, evaluate::operation::Operator::Neqv,
+    evaluate::operation::Operator::Max, evaluate::operation::Operator::Min};
+
 static bool IsValidAtomicUpdateOperation(
     const evaluate::operation::Operator &op) {
-  switch (op) {
-  case evaluate::operation::Operator::Add:
-  case evaluate::operation::Operator::Mul:
-  case evaluate::operation::Operator::Sub:
-  case evaluate::operation::Operator::Div:
-  case evaluate::operation::Operator::And:
-  case evaluate::operation::Operator::Or:
-  case evaluate::operation::Operator::Eqv:
-  case evaluate::operation::Operator::Neqv:
-  // 2909 intrinsic-procedure name is one of max, min, iand, ior, or ieor.
-  case evaluate::operation::Operator::Max:
-  case evaluate::operation::Operator::Min:
-    // Currently all get mapped to And, Or, and Neqv
-    // case evaluate::operation::Operator::Iand:
-    // case evaluate::operation::Operator::Ior:
-    // case evaluate::operation::Operator::Ieor:
-    return true;
-  case evaluate::operation::Operator::Convert:
-  case evaluate::operation::Operator::Identity:
-  default:
-    return false;
-  }
+  return validAccAtomicUpdateOperators.test(op);
 }
 
+// Couldn't reproduce this behavior with evaluate::UnwrapConvertedExpr which
+// is similar but only works within a single type category.
 static SomeExpr GetExprModuloConversion(const SomeExpr &expr) {
   const auto [op, args]{evaluate::GetTopLevelOperation(expr)};
   // Check: if it is a conversion then it must have at least one argument.
-  CHECK((op != evaluate::operation::Operator::Convert || args.size() >= 1) &&
+  CHECK(((op != evaluate::operation::Operator::Convert &&
+             op != evaluate::operation::Operator::Resize) ||
+            args.size() >= 1) &&
       "Invalid conversion operation");
-  if (op == evaluate::operation::Operator::Convert && args.size() == 1) {
+  if ((op == evaluate::operation::Operator::Convert ||
+          op == evaluate::operation::Operator::Resize) &&
+      args.size() >= 1) {
     return args[0];
   }
   return expr;
@@ -416,17 +404,15 @@ static SomeExpr GetExprModuloConversion(const SomeExpr &expr) {
 void AccStructureChecker::CheckAtomicUpdateStmt(
     const parser::AssignmentStmt &assign, const SomeExpr &updateVar,
     const SomeExpr *captureVar) {
-  static std::string construct{"update"};
-  CheckAtomicStmt(assign, construct);
+  CheckAtomicStmt(assign, "update");
   const auto &expr{std::get<parser::Expr>(assign.t)};
   const auto *rhs{GetExpr(context_, expr)};
   if (rhs) {
     const auto [op, args]{
         evaluate::GetTopLevelOperation(GetExprModuloConversion(*rhs))};
     if (!IsValidAtomicUpdateOperation(op)) {
       context_.Say(expr.source,
-          "Invalid atomic update operation, can only use: *, +, -, *, /, and, or, eqv, neqv, max, min, iand, ior, ieor"_err_en_US,
-          construct);
+          "Invalid atomic update operation, can only use: *, +, -, *, /, and, or, eqv, neqv, max, min, iand, ior, ieor"_err_en_US);
     } else {
       bool foundUpdateVar{false};
       for (const auto &arg : args) {
@@ -458,8 +444,7 @@ void AccStructureChecker::CheckAtomicUpdateStmt(
 void AccStructureChecker::CheckAtomicWriteStmt(
     const parser::AssignmentStmt &assign, const SomeExpr &updateVar,
     const SomeExpr *captureVar) {
-  static std::string construct{"write"};
-  CheckAtomicStmt(assign, construct);
+  CheckAtomicStmt(assign, "write");
   const auto &expr{std::get<parser::Expr>(assign.t)};
   const auto *rhs{GetExpr(context_, expr)};
   if (rhs) {
@@ -474,15 +459,16 @@ void AccStructureChecker::CheckAtomicWriteStmt(
 void AccStructureChecker::CheckAtomicCaptureStmt(
     const parser::AssignmentStmt &assign, const SomeExpr *updateVar,
     const SomeExpr &captureVar) {
-  static std::string construct{"capture"};
-  CheckAtomicStmt(assign, construct);
+  CheckAtomicStmt(assign, "capture");
 }
 
 void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
-  const Fortran::parser::AssignmentStmt &stmt1 =
-      std::get<Fortran::parser::AccAtomicCapture::Stmt1>(capture.t).v.statement;
-  const Fortran::parser::AssignmentStmt &stmt2 =
-      std::get<Fortran::parser::AccAtomicCapture::Stmt2>(capture.t).v.statement;
+  const Fortran::parser::AssignmentStmt &stmt1{
+      std::get<Fortran::parser::AccAtomicCapture::Stmt1>(capture.t)
+          .v.statement};
+  const Fortran::parser::AssignmentStmt &stmt2{
+      std::get<Fortran::parser::AccAtomicCapture::Stmt2>(capture.t)
+          .v.statement};
   const auto &var1{std::get<parser::Variable>(stmt1.t)};
   const auto &var2{std::get<parser::Variable>(stmt2.t)};
   const auto *lhs1{GetExpr(context_, var1)};
@@ -507,7 +493,7 @@ void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
   bool stmt2CapturesLhs1{*lhs1 == GetExprModuloConversion(*rhs2)};
   if (stmt1CapturesLhs2 && !stmt2CapturesLhs1) {
     if (*lhs2 == GetExprModuloConversion(*rhs2)) {
-      // a = b; b = b; Doesn't fit the spec;
+      // a = b; b = b: Doesn't fit the spec.
       context_.Say(std::get<parser::Verbatim>(capture.t).source,
           "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
       // TODO: Add attatchment that a = b seems to be a capture,
@@ -533,14 +519,14 @@ void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
       // Add attatchment that a = a is not considered an update,
       // but b = a seems to be a capture.
     } else {
-      // Take x = <expr>; v = x; as update; capture
+      // Take x = <expr>; v = x: as update; capture
       const auto &updateVar{*lhs1};
       const auto &captureVar{*lhs2};
       CheckAtomicUpdateStmt(stmt1, updateVar, &captureVar);
       CheckAtomicCaptureStmt(stmt2, &updateVar, captureVar);
     }
   } else if (stmt1CapturesLhs2 && stmt2CapturesLhs1) {
-    // x1 = x2; x2 = x1; Doesn't fit the spec;
+    // x1 = x2; x2 = x1; Doesn't fit the spec.
     context_.Say(std::get<parser::Verbatim>(capture.t).source,
         "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
     // TODO: Add attatchment that both assignments seem to be captures.
@@ -550,40 +536,33 @@ void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
         "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
     // TODO: Add attatchment that neither assignment seems to be a capture.
   }
-  return;
 }
 
 void AccStructureChecker::Enter(const parser::AccAtomicUpdate &x) {
   const auto &assign{
       std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement};
   const auto &var{std::get<parser::Variable>(assign.t)};
-  const auto *updateVar{GetExpr(context_, var)};
-  if (!updateVar) {
-    return;
+  if (const auto *updateVar{GetExpr(context_, var)}) {
+    CheckAtomicUpdateStmt(assign, *updateVar, /*captureVar=*/nullptr);
   }
-  CheckAtomicUpdateStmt(assign, *updateVar, nullptr);
 }
 
 void AccStructureChecker::Enter(const parser::AccAtomicWrite &x) {
   const auto &assign{
       std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement};
   const auto &var{std::get<parser::Variable>(assign.t)};
-  const auto *updateVar{GetExpr(context_, var)};
-  if (!updateVar) {
-    return;
+  if (const auto *updateVar{GetExpr(context_, var)}) {
+    CheckAtomicWriteStmt(assign, *updateVar, /*captureVar=*/nullptr);
   }
-  CheckAtomicWriteStmt(assign, *updateVar, nullptr);
 }
 
 void AccStructureChecker::Enter(const parser::AccAtomicRead &x) {
   const auto &assign{
       std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement};
   const auto &var{std::get<parser::Variable>(assign.t)};
-  const auto *captureVar{GetExpr(context_, var)};
-  if (!captureVar) {
-    return;
+  if (const auto *captureVar{GetExpr(context_, var)}) {
+    CheckAtomicCaptureStmt(assign, /*updateVar=*/nullptr, *captureVar);
   }
-  CheckAtomicCaptureStmt(assign, nullptr, *captureVar);
 }
 
 void AccStructureChecker::Enter(const parser::OpenACCCacheConstruct &x) {

flang/lib/Semantics/check-acc-structure.h

@@ -84,7 +84,7 @@ class AccStructureChecker
 
 private:
   void CheckAtomicStmt(
-      const parser::AssignmentStmt &assign, std::string &construct);
+      const parser::AssignmentStmt &assign, const std::string &construct);
   void CheckAtomicUpdateStmt(const parser::AssignmentStmt &assign,
       const SomeExpr &updateVar, const SomeExpr *captureVar);
   void CheckAtomicCaptureStmt(const parser::AssignmentStmt &assign,

flang/lib/Semantics/check-omp-atomic.cpp

@@ -197,7 +197,8 @@ static std::pair<parser::CharBlock, parser::CharBlock> SplitAssignmentSource(
 }
 
 static bool IsCheckForAssociated(const SomeExpr &cond) {
-  return GetTopLevelOperation(cond).first == operation::Operator::Associated;
+  return GetTopLevelOperationIgnoreResizing(cond).first ==
+      operation::Operator::Associated;
 }
 
 static bool IsMaybeAtomicWrite(const evaluate::Assignment &assign) {
@@ -607,7 +608,7 @@ void OmpStructureChecker::CheckAtomicUpdateAssignment(
   std::pair<operation::Operator, std::vector<SomeExpr>> top{
       operation::Operator::Unknown, {}};
   if (auto &&maybeInput{GetConvertInput(update.rhs)}) {
-    top = GetTopLevelOperation(*maybeInput);
+    top = GetTopLevelOperationIgnoreResizing(*maybeInput);
   }
   switch (top.first) {
   case operation::Operator::Add:
@@ -715,7 +716,7 @@ void OmpStructureChecker::CheckAtomicConditionalUpdateAssignment(
 
   CheckAtomicVariable(atom, alsrc);
 
-  auto top{GetTopLevelOperation(cond)};
+  auto top{GetTopLevelOperationIgnoreResizing(cond)};
   // Missing arguments to operations would have been diagnosed by now.
 
   switch (top.first) {