Ensure temporaries are passed in calls for parenthesized arguments

foo(x) and foo((x)) are semantically different. In the former, the dummy
argument is associated with x storage, while in the latter, the dummy
argument is associated with a temp initialized with x value.

This is an important distinction in calls like foo(x, (x)) where passing
x storage twice would be wrong, also to not propagate back potential changes
on the dummy.

Parentheses are not lowered to temps in lowering because we do not want
to create temp array sub-expression. Only a fir.no_reassoc is created
for parentheses by expression lowering.

This change keep this general behavior and add special handling in call
lowering to ensure a temp is created for parenthesized arguments.

Author: jeanPerier

flang/lib/Lower/ConvertExpr.cpp

@@ -1977,6 +1977,73 @@ class ScalarExprLowering {
     return call.getResult(0);
   }
 
+  /// Is this a variable wrapped in parentheses ?
+  template <typename A>
+  bool isParenthesizedVariable(const A &) {
+    return false;
+  }
+  template <typename T>
+  bool isParenthesizedVariable(const Fortran::evaluate::Expr<T> &expr) {
+    using ExprVariant = decltype(Fortran::evaluate::Expr<T>::u);
+    using Parentheses = Fortran::evaluate::Parentheses<T>;
+    if constexpr (Fortran::common::HasMember<Parentheses, ExprVariant>) {
+      if (const auto *parentheses = std::get_if<Parentheses>(&expr.u))
+        return Fortran::evaluate::IsVariable(parentheses->left());
+      return false;
+    } else {
+      return std::visit(
+          [&](const auto &x) { return isParenthesizedVariable(x); }, expr.u);
+    }
+  }
+
+  /// Like genExtAddr, but ensure the address returned is a temporary even if \p
+  /// expr is variable inside parentheses.
+  ExtValue genTempExtAddr(const Fortran::lower::SomeExpr &expr) {
+    // In general, genExtAddr might nor create a temp for variable inside
+    // parentheses to avoid creating array temporary in sub-expressions. It only
+    // ensure the sub-expression is not re-associated with other part of the
+    // expression. In the call semantic, there is a difference between expr and
+    // variable (see R1524). For expressions, a variable storage must not be
+    // argument associated since it could be modified inside the call, or the
+    // variable could also be modified by other means during the call.
+    if (!isParenthesizedVariable(expr))
+      return genExtAddr(expr);
+    if (expr.Rank() > 0)
+      return asArray(expr);
+    auto loc = getLoc();
+    return genExtValue(expr).match(
+        [&](const fir::CharBoxValue &boxChar) -> ExtValue {
+          return fir::factory::CharacterExprHelper{builder, loc}.createTempFrom(
+              boxChar);
+        },
+        [&](const fir::UnboxedValue &v) -> ExtValue {
+          auto type = v.getType();
+          mlir::Value value = v;
+          if (fir::isa_ref_type(type))
+            value = builder.create<fir::LoadOp>(loc, value);
+          auto temp = builder.createTemporary(loc, value.getType());
+          builder.create<fir::StoreOp>(loc, value, temp);
+          return temp;
+        },
+        [&](const fir::BoxValue &x) -> ExtValue {
+          // Derived type scalar that may be polymorphic.
+          assert(!x.hasRank() && x.isDerived());
+          if (x.isDerivedWithLengthParameters())
+            fir::emitFatalError(
+                loc, "making temps for derived type with length parameters");
+          // TODO: polymorphic aspects should be kept but for now the temp
+          // created always has the declared type.
+          auto var = fir::getBase(fir::factory::readBoxValue(builder, loc, x));
+          auto value = builder.create<fir::LoadOp>(loc, var);
+          auto temp = builder.createTemporary(loc, value.getType());
+          builder.create<fir::StoreOp>(loc, value, temp);
+          return temp;
+        },
+        [&](const auto &) -> ExtValue {
+          fir::emitFatalError(loc, "expr is not a scalar value");
+        });
+  }
+
   /// Lower a non-elemental procedure reference.
   ExtValue genRawProcedureRef(const Fortran::evaluate::ProcedureRef &procRef,
                               llvm::Optional<mlir::Type> resultType) {
@@ -2049,11 +2116,11 @@ class ScalarExprLowering {
           mutableModifiedByCall.emplace_back(std::move(mutableBox));
         continue;
       }
-
+      const bool actualArgIsVariable = Fortran::evaluate::IsVariable(*expr);
       if (arg.passBy == PassBy::BaseAddress || arg.passBy == PassBy::BoxChar) {
         auto argAddr = [&]() -> ExtValue {
           ExtValue baseAddr;
-          if (Fortran::evaluate::IsVariable(*expr) && expr->Rank() > 0) {
+          if (actualArgIsVariable && expr->Rank() > 0) {
             auto box = genBoxArg(*expr);
             if (!Fortran::evaluate::IsSimplyContiguous(
                     *expr, converter.getFoldingContext())) {
@@ -2070,8 +2137,12 @@ class ScalarExprLowering {
             // Contiguous: just use the box we created above!
             // This gets "unboxed" below, if needed.
             baseAddr = box;
-          } else
+          } else if (actualArgIsVariable) {
             baseAddr = genExtAddr(*expr);
+          } else {
+            // Make sure a variable address is not passed.
+            baseAddr = genTempExtAddr(*expr);
+          }
 
           // Scalar and contiguous expressions may be lowered to a fir.box,
           // either to account for potential polymorphism, or because lowering
@@ -2126,7 +2197,11 @@ class ScalarExprLowering {
           caller.placeInput(arg, builder.create<mlir::SelectOp>(
                                      loc, isAllocated, convertedBox, absent));
         } else {
-          auto box = builder.createBox(loc, genBoxArg(*expr));
+          // Make sure a variable address is only passed if the expression is
+          // actually a variable.
+          auto box = actualArgIsVariable
+                         ? builder.createBox(loc, genBoxArg(*expr))
+                         : builder.createBox(getLoc(), genTempExtAddr(*expr));
           caller.placeInput(arg, box);
         }
       } else if (arg.passBy == PassBy::AddressAndLength) {