HomePhabricator

[flang][msvc] Workaround 'forgotten' symbols in FoldOperation. NFC.
Concern Raisedd4a1db4f3fd7

Authored by Meinersbur on Sep 30 2020, 6:55 PM.

Description

[flang][msvc] Workaround 'forgotten' symbols in FoldOperation. NFC.

This resolves an issue where the Microsoft compiler 'forgets' symbols when using constexpr in a lambda in a templated function. The symbols are:

  1. The implicit lambda captures context and convert. Fix by making them explicit captures. The error message was:
fold-implementation.h(1220): error C2065: 'convert': undeclared identifier
  1. The function template argument FROMCAT. Fix by storing it in a temporary constexpr variable inside the function. The error message was:
fold-implementation.h(1216): error C2065: 'FROMCAT': undeclared identifier

This patch is part of the series to make flang compilable with MS Visual Studio http://lists.llvm.org/pipermail/flang-dev/2020-July/000448.html.

Reviewed By: klausler

Differential Revision: https://reviews.llvm.org/D88504

Details

Auditors
klausler
Committed
MeinersburSep 30 2020, 7:28 PM
Reviewer
klausler
Differential Revision
D88504: [flang][msvc] Workaround 'forgotten' symbols in FoldOperation. NFC.
Parents
rG6cd8511e5932: [WebAssembly] New-style command support
Branches
Unknown
Tags
Unknown

Event Timeline

klausler raised a concern with this commit.Oct 1 2020, 10:02 AM
klausler added a subscriber: klausler.

This change broke the build with clang.

Possible workaround (not tested with msvc):

diff --git a/flang/lib/Evaluate/fold-implementation.h b/flang/lib/Evaluate/fold-implementation.h
index bb5463e..e89219b 100644
--- a/flang/lib/Evaluate/fold-implementation.h
+++ b/flang/lib/Evaluate/fold-implementation.h
@@ -1154,11 +1154,17 @@ Expr<TO> FoldOperation(
   if (auto array{ApplyElementwise(context, convert)}) {
     return *array;
   }
+  struct {
+    FoldingContext &context;
+    Convert<TO, FROMCAT> &convert;
+  } msvcWorkaround{context, convert};
   return std::visit(
-      [&](auto &kindExpr) -> Expr<TO> {
+      [&msvcWorkaround](auto &kindExpr) -> Expr<TO> {
         using Operand = ResultType<decltype(kindExpr)>;
+        auto &convert{msvcWorkaround.convert};
         char buffer[64];
         if (auto value{GetScalarConstantValue<Operand>(kindExpr)}) {
+          FoldingContext &context{msvcWorkaround.context};
           if constexpr (TO::category == TypeCategory::Integer) {
             if constexpr (Operand::category == TypeCategory::Integer) {
               auto converted{Scalar<TO>::ConvertSigned(*value)};
This commit now has outstanding concerns.Oct 1 2020, 10:02 AM

This change broke the build with clang.

What exactly broke (version of clang, error message)?

I compiled this with clang myself, and it did work.

Possible workaround (not tested with msvc):

I tried this with msvc and unfortunately did not work.

  1. It seemed to confuse the context declared inside the lambda with the outside definition of context. Really weird.
  2. FROMCAT in the if constexpr is still unknown.

However, this worked:

diff --git a/flang/lib/Evaluate/fold-implementation.h b/flang/lib/Evaluate/fold-implementation.h
index bb5463e697fe..adb492c5f4d6 100644
--- a/flang/lib/Evaluate/fold-implementation.h
+++ b/flang/lib/Evaluate/fold-implementation.h
@@ -1154,16 +1154,25 @@ Expr<TO> FoldOperation(
   if (auto array{ApplyElementwise(context, convert)}) {
     return *array;
   }
+  struct {
+    FoldingContext &context;
+    Convert<TO, FROMCAT> &convert;
+  } msvcWorkaround{context, convert};
   return std::visit(
-      [&](auto &kindExpr) -> Expr<TO> {
+      [&msvcWorkaround](auto &kindExpr) -> Expr<TO> {
         using Operand = ResultType<decltype(kindExpr)>;
+        // This variable is a workaround for msvc which emits an error when
+        // using the FROMCAT template parameter below.
+        TypeCategory constexpr fromCat{FROMCAT};
+        auto &convert{msvcWorkaround.convert};
         char buffer[64];
         if (auto value{GetScalarConstantValue<Operand>(kindExpr)}) {
+          FoldingContext &ctx{msvcWorkaround.context};
           if constexpr (TO::category == TypeCategory::Integer) {
             if constexpr (Operand::category == TypeCategory::Integer) {
               auto converted{Scalar<TO>::ConvertSigned(*value)};
               if (converted.overflow) {
-                context.messages().Say(
+                ctx.messages().Say(
                     "INTEGER(%d) to INTEGER(%d) conversion overflowed"_en_US,
                     Operand::kind, TO::kind);
               }
@@ -1171,11 +1180,11 @@ Expr<TO> FoldOperation(
             } else if constexpr (Operand::category == TypeCategory::Real) {
               auto converted{value->template ToInteger<Scalar<TO>>()};
               if (converted.flags.test(RealFlag::InvalidArgument)) {
-                context.messages().Say(
+                ctx.messages().Say(
                     "REAL(%d) to INTEGER(%d) conversion: invalid argument"_en_US,
                     Operand::kind, TO::kind);
               } else if (converted.flags.test(RealFlag::Overflow)) {
-                context.messages().Say(
+                ctx.messages().Say(
                     "REAL(%d) to INTEGER(%d) conversion overflowed"_en_US,
                     Operand::kind, TO::kind);
               }
@@ -1188,7 +1197,7 @@ Expr<TO> FoldOperation(
                 std::snprintf(buffer, sizeof buffer,
                     "INTEGER(%d) to REAL(%d) conversion", Operand::kind,
                     TO::kind);
-                RealFlagWarnings(context, converted.flags, buffer);
+                RealFlagWarnings(ctx, converted.flags, buffer);
               }
               return ScalarConstantToExpr(std::move(converted.value));
             } else if constexpr (Operand::category == TypeCategory::Real) {
@@ -1196,9 +1205,9 @@ Expr<TO> FoldOperation(
               if (!converted.flags.empty()) {
                 std::snprintf(buffer, sizeof buffer,
                     "REAL(%d) to REAL(%d) conversion", Operand::kind, TO::kind);
-                RealFlagWarnings(context, converted.flags, buffer);
+                RealFlagWarnings(ctx, converted.flags, buffer);
               }
-              if (context.flushSubnormalsToZero()) {
+              if (ctx.flushSubnormalsToZero()) {
                 converted.value = converted.value.FlushSubnormalToZero();
               }
               return ScalarConstantToExpr(std::move(converted.value));
@@ -1213,7 +1222,7 @@ Expr<TO> FoldOperation(
             return Expr<TO>{value->IsTrue()};
           }
         } else if constexpr (std::is_same_v<Operand, TO> &&
-            FROMCAT != TypeCategory::Character) {
+            fromCat != TypeCategory::Character) {
           return std::move(kindExpr); // remove needless conversion
         }
         return Expr<TO>{std::move(convert)};

Does this fail for you because you compile with -Werror? The warning is a false positive in Clang: http://llvm.org/PR35450

Does this fail for you because you compile with -Werror? The warning is a false positive in Clang: http://llvm.org/PR35450

Yes, we always build f18 with -Werror.