`CMAKE_UNITY_BUILD` Broken as of 0e1c5df

[wavefunction91]

CMAKE_UNITY_BUILD allows for CMake to merge files to allow for faster compilation on some platforms. https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD.html

As of 0e1c5dfa458667cdc3f21a6e17f1544eb1fb5cef changes to syr2 to delegate to syr2k cause ODR violations for the internal Fortran API implementations when unity is enabled.

[mgates3]

I'm confused why this causes a problem. If I put

namespace blas {
namespace internal {

inline void syr2k( ... ) { ... }

}
}

into a header, say src/internal_syr2k.hh, and included it in both files (src/syr2.cc and src/syr2k.cc), that would be legal, correct? What we currently have seems equivalent to that, though putting code in a header to not repeat it would be better (hence the todo in the code).

Honestly, looking at what a unity build is, it seems very weird and fraught with errors. I don't get the advantage.

[wavefunction91]

As separate files, yes, this would be equivalent (modulo header guards) to the scenario you describe. In the unity build, the compiler sees a file of the form

// unity_x.cpp
#include "/some/path/syr2.cc"
#include "/some/path/syr2k.cc"

hence the ODR violations. Now if you did move the Fortran wrapper to a (guarded) header, it would proclude ODR violations. e.g.

// internal_syr2k.hh
#pragma once // This precludes ODR violations
namespace blas {
namespace internal {

inline void syr2k( ... ) { ... }

}
}

// syr2.cc
#include "internal_syr2k.hh"
...

// syr2k.cc
#include "internal_syr2k.hh"
...

unity_x.cpp would then compile.

As for why it's useful - it drastically increases compile times for large projects (like NWChemEx). We've seen that it speeds up blaspp/lapackpp builds as well (particularly lapackpp). It's not error prone if you're careful about ODR violations.

The issue on our end is that unity build is a huge benefit to our project, but the option is PROJECT-wide, which means it propagates to blaspp, etc when we build as a subproject. If you feel strongly about disabling it, you can disable by setting the UNITY_BUILD property for blaspp/lapackpp. But since the error is easily fixable, I don't see the advantage of not enabling it.

[mgates3]

Ah, now I see. We'll fix it somehow, either as you suggest or with a header.