[PATCH] D114557: [fir] Add base for runtime builder unittests

[Valentin Clement via Phabricator via llvm-commits]

clementval marked 4 inline comments as done.
clementval added inline comments.


================
Comment at: flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h:62
+template <>
+constexpr TypeBuilderFunc getModel<short int>() {
+  return [](mlir::MLIRContext *context) -> mlir::Type {
----------------
awarzynski wrote:
> rovka wrote:
> > (moving the discussion here from [[ https://reviews.llvm.org/D114460 | D114460 ]])
> > What I had in mind was either:
> > Option 1 - SFINAE:
> > 
> > ```
> > template <typename T>
> > static constexpr typenane std::enable_if_t<std::is_integral_v<T>, TypeBuilderFunc> getModel() {
> >   return [](mlir::MLIRContext *context) -> mlir::Type {
> >     return mlir::IntegerType::get(context, 8 * sizeof(T));
> >   };
> > }
> > // [...] lots of similar templates for is_floating_point, is_pointer etc etc
> > ```
> > or Option 2 - single template (probably easier to follow and maintain):
> > template <typename T>
> > static constexpr TypeBuilderFunc getModel() {
> >   return [](mlir::MLIRContext *context) -> mlir::Type {
> >      if constexpr (std::is_integral_v<T>) {
> >         return mlir::IntegerType::get(context, 8 * sizeof(T));
> >      } else // [...] branches for std::is_floating_point, is_pointer etc
> >   };
> > }
> +1
This can be a nice refactoring. I think this can be done after upstreaming. 


================
Comment at: flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h:141
+  return [](mlir::MLIRContext *context) -> mlir::Type {
+    return mlir::IntegerType::get(context, 8 * sizeof(std::size_t));
+  };
----------------
clementval wrote:
> kiranchandramohan wrote:
> > Nit: This is either my lack of familiarity or a bug.
> Yeah it looks odd. Let me double check that in fir-dev. 
I guess the initial author made the assumption that `std::size_t` == `long long`. I updated that. 


================
Comment at: flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h:27
+    mlir::FuncOp func = mlir::FuncOp::create(
+        loc, "func1", builder.getFunctionType(llvm::None, llvm::None));
+    auto *entryBlock = func.addEntryBlock();
----------------
awarzynski wrote:
> [nit] Could `func1` be replaced with something a bit more unique/descriptive? Perhaps `dummy_func_for_runtime_unit_tests`?
Not sure it improves anything since it is never queried or anything but if you like it better with another name then fine, 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114557