JeffersonLab / JANA2

Multi-threaded HENP Event Reconstruction
https://jeffersonlab.github.io/JANA2/
Other
6 stars 9 forks source link

fix: remove malignant alignas #239

Closed wdconinc closed 12 months ago

wdconinc commented 1 year ago

Removing this alignas override fixes https://github.com/JeffersonLab/JANA2/issues/238, and may resolve a number of mysterious issues in EICrecon over the past year:

I can confirm that removing the alignas override fixes one reproducible Eigen assert failure in a local copy. Removing alignas is also confirmed to avoids our need to include boost/container/small_vector.hpp before including JANA2 headers. (We fixed the avx/avx2 issue with Acts already by increasing separation between JANA2 factories and tracking algorithms.)

veprbl commented 1 year ago

This should be good - alignas is part of the standard https://en.cppreference.com/w/cpp/language/alignas now

faustus123 commented 1 year ago

This should be good - alignas is part of the standard https://en.cppreference.com/w/cpp/language/alignas now

I saw this too, but will admit to being unclear why the compiler is seeing it as undefined.

Regardless, This was all put in 6 years ago and I'm sure seemed like a good idea at the time. It needs to be removed. I would like to see the Get_CacheLineSize removed too unless someone can make an argument to keep it. Maybe @nathanwbrei can comment.

wdconinc commented 1 year ago

I saw this too, but will admit to being unclear why the compiler is seeing it as undefined.

Because it's not a macro. It's a keyword.

wdconinc commented 1 year ago

And even the fact that it's a keyword isn't important. At this point, only the preprocessor is active. It doesn't even know yet which language this is in. All it knows is that alignas is not a macro.

faustus123 commented 1 year ago

OK. So was it a macro at one point? How could it ever have worked then?

wdconinc commented 1 year ago

Maybe it never worked. It just disables alignas everywhere. When you are looking at an issue on Mac (old clang I assume), then that issue was resolved by this. It just also broke alignas everywhere else. Just took until now to have someone realize it.

nathanwbrei commented 12 months ago

Regardless, This was all put in 6 years ago and I'm sure seemed like a good idea at the time. It needs to be removed. I would like to see the Get_CacheLineSize removed too unless someone can make an argument to keep it. Maybe @nathanwbrei can comment.

Let's get rid of it! The code is outright incorrect, and manages to break an impressive number of things. If we discover we still need it, we should reimplement it as something along the lines of:

#ifdef __APPLE__
#define JANA2_ALIGNAS 
#else
#define JANA2_ALIGNAS alignas
#endif

Also, we should make the cache line size be a parameter we can set externally in one place, e.g. JANA2_CACHE_LINE_BYTES. Currently, we have Get_CacheLineSize(), CACHE_LINE_BYTES, and one hardcoded 64.