This is an archive of the discontinued LLVM Phabricator instance.

[fir] Add base for runtime builder unittests
ClosedPublic

Authored by clementval on Nov 24 2021, 11:53 AM.

Details

Summary

This patch adds the common base shared by builder runtime
unittests. It extracted from D114460 to make it easier to base other patches
on it.

Diff Detail

Event Timeline

clementval created this revision.Nov 24 2021, 11:53 AM
Herald added a project: Restricted Project. · View Herald Transcript
clementval requested review of this revision.Nov 24 2021, 11:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2021, 11:53 AM

Remove extra brace

Fix checkCallOpFromResultBox

rovka added inline comments.Nov 25 2021, 12:40 AM
flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h
87

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.

clementval marked an inline comment as done.Nov 25 2021, 3:39 AM
clementval added inline comments.
flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h
87

Added some comments.

clementval marked an inline comment as done.

Fix fir::LLVMPointerType::get due to missing builder.

rovka added inline comments.Nov 25 2021, 5:38 AM
flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
63

(moving the discussion here from 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
};

}

Can flang/lib/Lower/RTBuilder.h be deleted now?

LGTM. Please wait for a day if there are comments outstanding in this patch or before it was refactored.

flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
142

Nit: This is either my lack of familiarity or a bug.

flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h
78

Nit: would it make sense to check the return value or return type since some runtime functions return an mlir::value and others return in the descriptor passed as an argument?

This revision is now accepted and ready to land.Nov 25 2021, 6:00 AM

Can flang/lib/Lower/RTBuilder.h be deleted now?

Yeah I think lowering can use this more complete version. It should be refactored in fir-dev before upstreaming.

clementval added inline comments.Nov 25 2021, 6:12 AM
flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
142

Yeah it looks odd. Let me double check that in fir-dev.

flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h
78

Of course we can check more stuff. I was just trying to find a trade off between no test and time spent on upstreaming this.

awarzynski added inline comments.Nov 25 2021, 7:55 AM
flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
63

+1

flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h
28

[nit] Could func1 be replaced with something a bit more unique/descriptive? Perhaps dummy_func_for_runtime_unit_tests?

113

This way you could avoid else further down. This is a nit.

115

Can this dyn_cast fail? Perhaps add EXPECT_NE(nullptr, convOp);?

clementval marked 4 inline comments as done.Nov 25 2021, 12:27 PM
clementval added inline comments.
flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
63

This can be a nice refactoring. I think this can be done after upstreaming.

142

I guess the initial author made the assumption that std::size_t == long long. I updated that.

flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h
28

Not sure it improves anything since it is never queried or anything but if you like it better with another name then fine,

clementval marked an inline comment as done.

ddress review comments

rovka accepted this revision.Nov 26 2021, 12:35 AM
rovka added inline comments.
flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
63

Ok, SGTM.

flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h
87

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).

clementval marked 4 inline comments as done.Nov 26 2021, 9:47 AM
clementval added inline comments.
flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
63
This revision was automatically updated to reflect the committed changes.