Add VecCore external #497
Add VecCore external #497
Conversation
amadio
force-pushed
the
veccore-external
branch
from
ad70db1 to
1e508da
Compare
amadio
force-pushed
the
veccore-external
branch
from
1e508da to
0764ca1
Compare
|
@phsft-bot build! |
|
Starting build on |
| VERSION_VAR Vc_VERSION) | ||
| endif() | ||
|
|
||
| if (vc OR builtin_vc) |
There was a problem hiding this comment.
@amadio, it seems that if I configure with -Dbuiltin_veccore we don't get builtin_vc on, thus VECCORE_ENABLE_VC macro doesn't expanded. In turn, this leads in not being able to compile Math_vectypes.hxx.
@xvallspl and I were discussing and we can diagnose this either in the cxx file or here. Alternatively, we should just enable builtin_vc, saving time and efforts :)
There was a problem hiding this comment.
I made it such that builtin_veccore forces builtin_vc to be ON if vc is ON. However, it should be possible to use VecCore without Vc (that's the whole point of having VecCore), so I'm against forcing Vc and VecCore to be locked to be the same, or we will never be able to change the backend later to be either Scalar or UME::SIMD. The macro VECCORE_ENABLE_VC is in ${VecCore_DEFINITIONS} when the Vc backend is enabled, so targets should rely on that rather than turning it on by hand.
There was a problem hiding this comment.
Concerning ${VecCore_DEFINITIONS} I just committed a change to hide this inside Math_vectypes.h otherwise is unworkable. It means that enybody including a ROOT Math header will need to define VECCORE_ENABLE_VC of ROOT was built with Vc and VecCore enabled.
There was a problem hiding this comment.
Hi @peremato, I saw the commit and agree with you that everybody having to define it is not optimal, but the current solution is not great either, because it only works when both VecCore and Vc are enabled. We will need to fix it later to be able to use at least the UME::SIMD backend, which works much better on KNL. At some point, we should require that new developments work with both UME::SIMD and Vc, otherwise there is no point in not just using Vc directly. I think that using ${VecCore_DEFINITIONS} in combination with #cmakedefine to create a configuration header is the way to go in this case.
I would prefer to wait until developments using VecCore within ROOT are more stable. In its current form, the targets are not setup correctly, so if you configure ROOT with VecCore=ON and Vc=OFF things are likely to break. That should definitely not be the case in the long term. Either we use Vc directly, or we rely on VecCore and write code that won't break unless Vc is turned ON. |
This pull request addresses Pere's comments and adds some more changes like a simple test for VecCore functionality and better support for latest VecCore changes (from version 0.3.2 to 0.4.0). It should be ready for merging, but I'd like to see all tests pass first. I tested on my own machine with several combinations of
vc=ON/OFF,builtin_vc=ON/OFFandveccore=ON/OFF,builtin_veccore=ON/OFF. It should all work properly. However, since external VecCore cannot find a builtin Vc, requestingbuiltin_vc=ONandveccore=ONwill triggerbuiltin_veccore=ONautomatically. Please feel free to to make more suggestions for extra changes if necessary.