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 etcor 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:
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 etcor 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 };}