search

halide / Halide

a language for fast, portable data-parallel computation
https://halide-lang.org
Other
5.86k stars 1.07k forks source link

src/runtime does a lot of `#include "some.cpp"` hackery. Can we do better? #6604

Open steven-johnson opened 2 years ago

steven-johnson commented 2 years ago

This isn't broken per se, but code that includes another .cpp file really isn't great form and frankly makes my teeth hurt, but we do it about a dozen times in our runtime. Surely we can do better using modern C++ techniques, somehow?

slomp commented 2 years ago

I did rely on some of this pattern with windows_d3d12compute_x86.cpp and windows_d3d12compute_arm.cpp. They serve as stubs to prevent a runtime from being generated for 32bits targets (by producing an empty runtime), and to set some preprocessor flags (HALIDE_D3D12_PLATFORM) that can be used by the actual implementation in d3d12compute.cpp. It turns out we're not really using that preprocessor flag for anything critical anymore, so we could technically remove this stubbing from the d3d12 runtimes, at least.

As for the other cases, I am not familiar with any modern C++ trick we could use to eliminate this. We could always rename the innermost "cpp" files to "inl" or something, but that's just putting lipstick on a pig.

alexreinking commented 2 years ago

We could always rename the innermost "cpp" files to "inl" or something, but that's just putting lipstick on a pig.

At the very least that more clearly communicates that those files are included fragments rather than standalone. Such a change would be pure win from the status quo, so we can do that without excluding the possibility of doing something better still down the line.

slomp commented 2 years ago

Yup, won't hurt to do that since it's a bit more idiomatic. I'll test it tomorrow and open a PR 👍