[RVec] Forward declare VDT functions in RVec.hxx header

[guitargeek]

Introduce forward declarations of the vdt functions int RVec.hxx so we don't rely on the <vdt/vdtMath.h> header always being in the include path.

Closes #9736, because now there is no builtin library left that ends up included in ROOT modules.

[vgvassilev]

Should that go in vdt itself as vdt_fwd.h?

[guitargeek]

I don't think it's worth it, these forward declaration headers make more sense if the types are complicated and expected to change sometimes (e.g. template types where template arguments are expected to be added).

The VDT function signatures have not changed in 14 years though, so I wouldn't consider a forward declaration header necessary.

[guitargeek]

But I think that should be up to the maintainer of vdt, @dpiparo

[vgvassilev]

Yes, but that section can go out of sync if upstream changes. Logistically putting it in the vdt library is the right layering approach afaict.

[guitargeek]

But then it doesn't solve the problem anymore: the whole point of this change is to make RVec.hxx work if vdt is not in the include path, for most common case where you don't need the fast_* functions. Like this, it also doesn't end up in the modules.

In the case where you need VDT, you can simply include it yourself. This follows the "only pay for what you use" model that we're generally trying to follow now.

[vgvassilev]

I realize that I do not understand the whole idiom. Eg: #define RVEC_VDT_UNARY_FUNCTION(F) RVEC_UNARY_FUNCTION(F, vdt::F). Why do we carry all these in the public header file? If they are implementation defined we might be able to put that logic in a local header file.

[guitargeek]

Yes, that should work as you designed it even if the header is in vdt, no? In there if your forward declaration header is standalone we won’t include it as part of the modules setup, either.

Also the "standalone vdt forward declaration header" would have to be in a place where ROOT can find it at runtime, also increasing the include path surface that this PR tries to minimize to begin with. Alternatively, ROOT would have to ship it, which goes against the direction of making ROOT dependencies more modular.

If you strongly feel that this your approach is better I am fine with it. Just looks like a layering and invalidation problem that we might have. Probably vdt does not change a lot and the benefit is marginal…

Thanks!

I realize that I do not understand the whole idiom. Eg: #define RVEC_VDT_UNARY_FUNCTION(F) RVEC_UNARY_FUNCTION(F, vdt::F). Why do we carry all these in the public header file? If they are implementation defined we might be able to put that logic in a local header file.

These RVEC_VDT_UNARY_FUNCTION(F) RVEC_UNARY_FUNCTION(F, vdt::F) definitions available in the public headers are required for RVec to work. This is what they resolve to:

  template <typename T>
  RVec<PromoteType<T>> fast_expf(const RVec<T> &v) {
     RVec<PromoteType<T>> ret(v.size());
     std::transform(v.begin(), v.end(), ret.begin(),
                    [](const T &x) { return vdt::fast_expf(x); });
     return ret;
  }

For the most common types float and double, the template instantiations are externalized to the RVec.cxx translation unit by these RVEC_EXTERN_VDT_UNARY_FUNCTION macros. But we still need to have the definitions in the public header for the general T.

[vgvassilev]

Also the "standalone vdt forward declaration header" would have to be in a place where ROOT can find it at runtime, also increasing the include path surface that this PR tries to minimize to begin with. Alternatively, ROOT would have to ship it, which goes against the direction of making ROOT dependencies more modular.

I do not get that part. We can still have the fwd declaring header in vdt and only include it in R_HAS_VDT context. We can make it even more convenient by adding an X-macro facility that will expand for each macro defined in the RVec.hxx file.

[guitargeek]

vdt, including the supposed fwd declaration header that you suggest to add, is not part of ROOT. So it has to be in the include path if you use ROOT master, which is a nuisance. R_HAS_VDT is only telling you if ROOT was built with the vdt features, not if its header is available at runtime.

[andresailer]

I.e. one can get this error (https://cdash.cern.ch/tests/13049607)

In file included from /build/jenkins/workspace/lcg_nightly_pipeline/build/projects/ROOT-HEAD/src/ROOT/HEAD/roottest/root/io/newstl/TestOutput.h:15:
/build/jenkins/workspace/lcg_nightly_pipeline/build/projects/ROOT-HEAD/src/ROOT-HEAD-build/include/ROOT/RVec.hxx:50:10: fatal error: 'vdt/vdtMath.h' file not found
#include <vdt/vdtMath.h>
         ^~~~~~~~~~~~~~~

[github-actions]

Test Results

    23 files      23 suites   3d 18h 53m 22s ⏱️
 3 877 tests  3 875 ✅ 0 💤 2 ❌
79 862 runs  79 855 ✅ 3 💤 4 ❌

For more details on these failures, see this check.

Results for commit 3a9f593.

♻️ This comment has been updated with latest results.

[hageboeck]

I think this is might be the wrong approach. If ROOT says it has vdt, it should have vdt, and not only broken forward declarations.

What we could do is check whether the header can be included using __has_include, and then perform the inclusion.
If __has_include is false, the functions get disabled. Forcing the user to include a header that they might not know and might have trouble finding seems to break most expectations. An equivalent example would be

#include <vector>
#include <iterator>

in order to call vector.begin();. It should be self-contained.

[hageboeck]

If I understand correctly, what you have in mind is to stabilise the interpreter?
In that case, would forward declaring the functions when the header can't be included do the trick, and including it when it is found?

In general, things that are declared should also work, but maybe we should make an exception if it prevents interpreter startup.

[guitargeek]

If I understand correctly, what you have in mind is to stabilise the interpreter?

Not at all what I have in mind! Let me try to state the point better, as I obviously explained it badly the first time 😆

The core thing I keep failing to get across is that this change separates two things that are currently entangled:

1. Do the fast_* methods in RVec exist? This is determined by how ROOT was built (R__HAS_VDT), baked into RConfigure.h. Single source of truth on whether VDT was available at build time.
2. Are vdt's headers on the include path of whoever #includes RVec.hxx at runtime (or downstream build time if you use ROOT as a library)? This is entirely separate question about the downstream build, in particular if ROOT was built with vdt=ON and builtin_vdt=OFF, in which case the availability of VDT headers might not be the same at build time and runtime.

With builtin_vdt=OFF, vdt is a private system dependency of libvecops. So (1) can be true while
(2) is false, and today that combination means someone who only wants Sum/Filter/Map (i.e. ~95% of RDataFrame users, who never touch fast_*) gets fatal error: 'vdt/vdtMath.h' file not found for a dependency they never asked for. That's the bug I want to fix: make RVec.hxx self-contained so vdt is needed only at link time, and only if you actually call fast_*.

On "if ROOT says it has vdt, it should have vdt, not broken forward declarations": agreed on the sentiment, and I don't think these are broken or can easily break. They can't silently drift, because RVec.cxx compiles against the real <vdt/vdtMath.h> when it emits the explicit float/double instantiations. If a signature ever changed, the ROOT nightly build fails with a conflicting-declaration error. And these signatures haven't moved in 14 years, so it's not really something that we expect to drift anyway.

Worth noting we already do exactly this pattern in other places, for the same reason:

• tbb in core/imt/inc/ROOT/RTaskArena.hxx: the header comment literally says
  "Necessary in order to keep tbb away from ROOT headers."
• PyObject (struct _object; typedef _object PyObject;) in TPython.h, etc, so you can build against those without Python's dev headers on your include path, even though ROOT very much "has Python"

On "forcing the user to include a header they might not know": well yes that's true, as you see in the tutorial change I had to make, you might need to include VDT. But that only affects the rare power users that use the fast_* functions in the interpreter (in compiled code, they will resolve to the extern definitions in RVec.cxx). And the only place to learn about these is exactly from the tutorial that will tell you to include vdt now. So this is the small sacrifice I'd be willing to make to increase relocatability to systems without VDT.

On __has_include: I'd push back on that one specifically, because it re-entangles (1) and (2): it makes the existence of fast_* depend on the downstream include path (just like Vassil's previous suggestion with the forward declaration header in VDT). The same ROOT install would then offer fast_* in one translation unit and not another depending on -I flags, and it puts the vdt header back into the vecops module. Which is #9736 again. The forward-decl approach keeps one deterministic source of truth (R__HAS_VDT) and makes the header compile everywhere.

Does this framing change how you see it?

[vgvassilev]

and it puts the vdt header back into the vecops module

Only if vdt does not have its own pcm file which would be the "right" layering of the system. Perhaps adding a modulemap for vdt is not a bad way out...

[guitargeek]

I think we're talking past each other here: the situation I want to improve is people compiling their own code against ROOT with a plain compiler that isn't going through ROOT's pcms at all. For them, #include <vdt/vdtMath.h> in RVec.hxx is just an annoying include if they don't use the fast_* functions that fails when the vdt headers aren't on the machine: the vdt=ON builtin_vdt=OFF + vdt-not-installed case. A modulemap doesn't help there, I think modules organize the declarations when the header is available, they don't produce it when it's absent. The forward declarations are what make RVec.hxx truly self-contained for that plain-compile case when one doesn't want to use the VDT features.

I guess the reason you circle back on modules and layering here is that you opened #9736, which isn't what this PR addresses directly. But I'd argue that issue is now better closed than met with new machinery: the concrete case it was opened for, Vc leaking into a dictionary, is gone from the codebase, and this PR removes the vdt in RVec.hxx case too. I'd rather not add a diagnostic to guard a misconfiguration that can no longer happen. That's maintenance cost for a case that doesn't exist. Could that be where we're talking past each other? What would be your preferred way to address #9736 at this point?

[guitargeek]

What about disconnecting this PR from #9736. Would that be okay for you? The problem I'm trying to solve here has nothing to do with modules, as I said, and more with using RVec.hxx in compiled code when the VDT headers aren't on the include path. Would it be okay to go ahead with this PR and leave #9736 open for the general case of modules depending on external headers (even though it's hypothetical for ROOT at this point, it might come back)?

[vgvassilev]

The issue is more subtle I guess. However, with modules and when vdt is on, at runtime when we start root we have to provide the include path for the vdt header files. That’s the underlying issue we are trying to solve right?

If so, then it is the same and worse for modules because instead of getting error: file not found, ROOT crashes.

What I am trying to say (and perhaps missing something) is that the forward declarations solve it nicely by dissolving the relationship between the headers of root and vdt. What I was/am worried about is that forward declaration logic is disconnected from vdt and if vdt adds a new function we don’t see it automatically. Indeed that does not make things worse from status quo as we also enumerate all we know about.

So, my review was mostly on improving the invalidation mechanism and was not really blocking this fix. My wish is if we can get rid of the vdt declarations from that header altogether would be the best and move everything to the implementation but I do not see a possible way forward.

As I said originally, the current approach is landable because it is an improvement over the status quo.

[hageboeck]

Not at all what I have in mind! Let me try to state the point better, as I obviously explained it badly the first time 😆

The core thing I keep failing to get across is that this change separates two things that are currently entangled:

1. Do the fast_* methods in RVec exist? This is determined by how ROOT was built (R__HAS_VDT), baked into RConfigure.h. Single source of truth on whether VDT was available at build time.
2. Are vdt's headers on the include path of whoever #includes RVec.hxx at runtime (or downstream build time if you use ROOT as a library)? This is entirely separate question about the downstream build, in particular if ROOT was built with vdt=ON and builtin_vdt=OFF, in which case the availability of VDT headers might not be the same at build time and runtime.

With builtin_vdt=OFF, vdt is a private system dependency of libvecops. So (1) can be true while (2) is false, and today that combination means someone who only wants Sum/Filter/Map (i.e. ~95% of RDataFrame users, who never touch fast_*) gets fatal error: 'vdt/vdtMath.h' file not found for a dependency they never asked for. That's the bug I want to fix: make RVec.hxx self-contained so vdt is needed only at link time, and only if you actually call fast_*.

OK, I understand better, but there is maybe a problem. Most of the vdt functions are in the header, and they are supposed to get inlined to allow for auto-vec throughout entire loops. Forward-declaring them breaks vdt, because you can't link to these. I see a few ways forward:

1. We use

#if __has_include ( "vdt/vdtmath.h" )
#include "vdt/vdtmath.h
#endif

and later:
#if __has_include ( "vdt/vdtmath.h" )
RVEC_UNARY_FUNCTION(vdt::fast_xxx( ... ))
#endif

If the header is in the include path, ROOT works with vdt. If it's not, the functions don't get declared. Does this solve the problem of the 95%?

2a) We outline all vdt usage into libROOTVecOps. The problem here is that we can't do it for every possible template argument, but that doesn't matter because vdt anyway only supports float/double IIRC.

2b) We change RVec to use the library functions of vdt. Note that currently, we ignore the vdt library throughout ROOT and only inline the functions from the headers. For RVec in particular, however, we could move over to the library vdt functions at the cost of not being able to inline them. Now, you can forward-declare them, and we don't need vdt headers the are publicly visible anywhere in ROOT. The usage of VDT can now become a PRIVATE cmake dependency, and we don't need to install the headers. We still need the library, unless we link statically.

3. We remove the vdt functions from RVec entirely. Honestly, I doubt that they make a big difference here, because how many times are they actually used?

Looking at what I wrote, I would go for 1. or for 2.a. What do you think?