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

[Diana Picus via Phabricator via llvm-commits]

rovka accepted this revision.
rovka 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 {
----------------
clementval wrote:
> 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. 
Ok, SGTM.


================
Comment at: flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h:86
+
+static void checkCallOpFromResultBox(mlir::Value result,
+    llvm::StringRef fctName, unsigned nbArgs, bool addLocArgs = true) {
----------------
clementval wrote:
> rovka wrote:
> > I think this needs a comment about exactly what it's checking (e.g. some fir snippet), so it's easier for people to know when to use it.
> Added some comments. 
Thanks, I think it's much clearer now (although the use of 'result' is still a bit misleading, since you're actually starting from a function argument rather than the actual result, which would be %0 in the snippet; I don't have a better name for it though, so feel free to leave it as is).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114557