[flang][msvc] Work around if constexpr (false) evaluation. NFC.

[Meinersbur]

Summary

MSVC tries to expand templates that are in the false-branch of a if constexpr construct. In this case, the condition checks whether a tuple has at least one element and then is trying to access it using std::get<0>, which fails when the tuple has 0 elements.

The workaround is to extract that case into a separate method.

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.

[Meinersbur]

Microsoft bug report: https://developercommunity.visualstudio.com/content/problem/1185857/if-constexpr-false-body-evaluated-in-variadic-temp.html

[klausler]

Is there any effect on the performance of the parser on large Fortran source files when it's built with non-Microsoft C++ compilers?

[Meinersbur]

The function should be inlined, in which case there is no performance difference.

[klausler]

In D87728#2275525, @Meinersbur wrote:

The function should be inlined, in which case there is no performance difference.

Sure, but have you measured the performance and verified that there's no penalty in gcc & clang builds for this MSVC work-around? I'm worried that we'll get to the end of this MSVC bug work-around process, notice that the Linux builds of f18 have lost performance, and somebody else is going to have to figure out which of these patches needs to be reworked to avoid the penalty. Are you able to measure f18 parsing performance? If not, let me know.

[Meinersbur]

Do you have a suggestion for a large Fortran file that I could measure? I think my measurements would not be accurate enough to show a difference between inlining and non-inlining of this function. If you know how to do it yourself, I would be grateful.

[klausler]

In D87728#2277006, @Meinersbur wrote:

Do you have a suggestion for a large Fortran file that I could measure? I think my measurements would not be accurate enough to show a difference between inlining and non-inlining of this function. If you know how to do it yourself, I would be grateful.

Whether or not the measurements differ more than the margin of error is essentially the problem that I'm worried about. The f18 parser is a stressful case for C++ compilers and inline expansion is not guaranteed. The template being changed here is the one that's used to generate every node in the parse tree, so each cycle matters.

We'll see if we can find some good examples of Fortran source files that are unencumbered and suitable for inclusion in the test base. One nice thing about Fortran is that one may concatenate a lot of source code into a single source file as long as module dependency order is observed, so one can build up very large single source files that can expose parsing performance changes. In the meantime, go ahead with your changes, and I'll keep an eye on performance here and let you know if I notice a drop, and we'll deal with it if it happens via conditional code or something.

[Meinersbur]

Thank you.

flang/lib/Parser/basic-parsers.h

public:
   using resultType = RESULT;
   constexpr ApplyConstructor(const ApplyConstructor &) = default;
   constexpr explicit ApplyConstructor(PARSER... p) : parsers_{p...} {}
   std::optional<resultType> Parse(ParseState &state) const {
     if constexpr (sizeof...(PARSER) == 0) {
       return RESULT{};
     } else {
       if constexpr (sizeof...(PARSER) == 1) {
-        if constexpr (std::is_same_v<Success, typename PARSER::resultType...>) {
-          if (std::get<0>(parsers_).Parse(state)) {
-            return RESULT{};
-          }
-        } else if (auto arg{std::get<0>(parsers_).Parse(state)}) {
-          return RESULT{std::move(*arg)};
-        }
+        return ParseOne(state);
       } else {
         ApplyArgs<PARSER...> results;
         using Sequence = std::index_sequence_for<PARSER...>;
         if (ApplyHelperArgs(parsers_, results, state, Sequence{})) {
           return ApplyHelperConstructor<RESULT, PARSER...>(
               std::move(results), Sequence{});
         }
       }
       return std::nullopt;
     }
   }
 
 private:
+  std::optional<resultType> ParseOne(ParseState &state) const {
+    if constexpr (std::is_same_v<Success, typename PARSER::resultType...>) {
+      if (std::get<0>(parsers_).Parse(state)) {
+        return RESULT{};
+      }
+    } else if (auto arg{std::get<0>(parsers_).Parse(state)}) {
+      return RESULT{std::move(*arg)};
+    }
+    return std::nullopt;
+  }
+
   const std::tuple<PARSER...> parsers_;
 };
 
 template <typename RESULT, typename... PARSER>
 inline constexpr auto construct(PARSER... p) {
   return ApplyConstructor<RESULT, PARSER...>{p...};
 }