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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski 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 {
----------------
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


================
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();
----------------
[nit] Could `func1` be replaced with something a bit more unique/descriptive? Perhaps `dummy_func_for_runtime_unit_tests`?


================
Comment at: flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h:112
+  if (mlir::isa<fir::CallOp>(*u)) {
+    checkCallOp(*u, fctName, nbArgs, addLocArgs);
+  } else {
----------------
This way you could avoid `else` further down. This is a nit.


================
Comment at: flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h:114
+  } else {
+    auto convOp = mlir::dyn_cast<fir::ConvertOp>(*u);
+    checkCallOpFromResultBox(convOp.getResult(), fctName, nbArgs, addLocArgs);
----------------
Can this `dyn_cast` fail? Perhaps add `EXPECT_NE(nullptr, convOp);`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114557