[flang-commits] [PATCH] D88981: [flang] Rework host runtime folding and enable REAL(2) folding with it.

[Peter Klausler via Phabricator via flang-commits]

klausler accepted this revision.
klausler added a comment.
This revision is now accepted and ready to land.

There's a lot of work here and it's quite well done; thank you.

I suggest that you add meinersbur as a reviewer to check that this code doesn't run afoul of some MSVC bug.



================
Comment at: flang/include/flang/Common/static-multimap-view.h:42
+  template <std::size_t N>
+  constexpr StaticMultimapView(const V (&array)[N])
+      : range_{&array[0], &array[0] + N} {}
----------------
The argument could be `const std::array<V,N> &array`.


================
Comment at: flang/include/flang/Common/static-multimap-view.h:47
+
+  // Assume array is sorted.
+  // TODO make it a log(n) search based on sorted property
----------------
This could be a constexpr check in the constructor.


================
Comment at: flang/include/flang/Common/static-multimap-view.h:50
+  // std::equal_range will be constexpr in C++20 only.
+  constexpr Range getRange(const Key &key) const {
+    bool matched{false};
----------------
We use `GetRange` naming in Common.

Could GetRange be private?


================
Comment at: flang/include/flang/Common/static-multimap-view.h:85
+} // namespace Fortran::common
+#endif // FORTRAN_COMMON_STATIC_MULTIMAP_VIEW_H_
----------------
C++20 has `constexpr` versions of `std::equal_range`, `std::lower_bound`, `std::binary_search`, &c.

If you could use them now, would this new class be necessary?

If we will eventually use them, maybe what's needed here would be just a place-holding `constexpr` function or two, possibly conditional on the C++ language standard version.


================
Comment at: flang/include/flang/Evaluate/common.h:11
 #define FORTRAN_EVALUATE_COMMON_H_
 
 #include "flang/Common/Fortran.h"
----------------
Does removing this member from the folding context make them cheap to construct again?


================
Comment at: flang/lib/Evaluate/fold-implementation.h:88
+  }
+  return {};
+}
----------------
`return std::nullopt;` is a little more clear, I think.


================
Comment at: flang/lib/Evaluate/intrinsics-library.cpp:141
+template <typename FuncType, typename TR, typename... TA, size_t... I>
+static Expr<SomeType> applyHostFunctionHelper(FuncType func,
+    FoldingContext &context, std::vector<Expr<SomeType>> &&args,
----------------
Please capitalize the names of these functions; thanks.


================
Comment at: flang/lib/Lower/IntrinsicCall.cpp:507
                                      Fortran::lower::FirOpBuilder &builder,
                                      const RuntimeFunction (&lib)[N],
                                      llvm::StringRef name,
----------------
I know that this isn't part of this change, but maybe this argument should be a `const std::array<> &`.


================
Comment at: flang/unittests/Evaluate/folding.cpp:56
       // Biggest IEEE 32bits subnormal power of two
       host::HostType<R4> input1{5.87747175411144e-39};
       const Scalar<R4> x1{host::CastHostToFortran<R4>(input1)};
----------------
I wonder if this constant should be checked; I worry about C++ compilation on a target machine that likes to flush subnormals to zero.  Maybe a hexadecimal floating point constant should be used, or even a reinterpreted 0x00400000.

(The value should be 5.877471754111438E-39 if you want a `double` constant that converts to and back from the desired value; the value you have above is slightly too large and converts to a double with a lower bit set that's lost in the conversion to `float` in most rounding modes, but why take chances.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88981/new/

https://reviews.llvm.org/D88981