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

[Jean Perier via Phabricator via flang-commits]

jeanPerier added a comment.

Thanks for the reviews, I have updated the patch where I could.



================
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} {}
----------------
klausler wrote:
> The argument could be `const std::array<V,N> &array`.
I do not know a way to do that without having the tables declared as std::array which I find annoying because you cannot nicely delegate the array length to the compiler (e.g. `T x[]{...}` vs `std::array<T, /* need to compute/ guess that */> x{...}`).


================
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
----------------
klausler wrote:
> This could be a constexpr check in the constructor.
Thanks, I tried hard to make that happen in the ctor/a static Create helper, but there is no way I could find to have it constexpr check that way since the constexpr aspect does not actually guarantee that this will be called with a constexpr array at compile time, so static_assert are rejected.
Instead I added checks after each instantiation. That's still better than nothing and actually caught a bessel_y0/y1 not sorted correctly in pgmath header.


================
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};
----------------
klausler wrote:
> We use `GetRange` naming in Common.
> 
> Could GetRange be private?
`GetRange` was removed.


================
Comment at: flang/include/flang/Common/static-multimap-view.h:85
+} // namespace Fortran::common
+#endif // FORTRAN_COMMON_STATIC_MULTIMAP_VIEW_H_
----------------
klausler wrote:
> 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.
What is mainly missing for me here is a constexpr `std::is_storted` (also C++20), as well as something like `std::span` (C++20) that (from what I understand) can "erase" the lenght  from the base container type and still allow constexpr iteration on it.
`GetRange` used to be called in constexpr in lowering to get the runtime but that is not the case anymore, so I already switched to std::equal_range and remove some other member functions that removed ~20 lines of boilerplate.


================
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)};
----------------
klausler wrote:
> 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.)
Thanks for the tip, I used the raw evaluate::Real constructor to avoid any issue with C++ compilers here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88981