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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h | ||
---|---|---|
87 | Added some comments. |
flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h | ||
---|---|---|
63 | (moving the discussion here from D114460) 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): 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? |
Yeah I think lowering can use this more complete version. It should be refactored in fir-dev before upstreaming.
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. |
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);? |
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, |
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). |
flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h | ||
---|---|---|
63 | Added to the board https://github.com/flang-compiler/f18-llvm-project/issues/1272 |
(moving the discussion here from D114460)
What I had in mind was either:
Option 1 - SFINAE:
or Option 2 - single template (probably easier to follow and maintain):
template <typename T>
static constexpr TypeBuilderFunc getModel() {
}