Open chriselrod opened 2 months ago
@llvm/issue-subscribers-clang-modules
Author: Chris Elrod (chriselrod)
Here is the minimal example using immintrin.h
:
hello.cxxm
:
#ifndef USE_HEADERS
module;
#endif
#include <concepts>
#include <immintrin.h>
#include <iostream>
#ifndef USE_HEADERS
export module Hello;
export {
#endif
void hello() { std::cout << "Hello World!\n"; }
template <typename T> auto vload128(const T *p) {
if constexpr (std::same_as<T, float>) {
return _mm_loadu_ps(p);
} else if constexpr (std::same_as<T, double>) {
return _mm_loadu_pd(p);
} else {
return _mm_loadu_si128(reinterpret_cast<const __m128i *>(p));
}
}
#ifndef USE_HEADERS
}
#endif
use.cpp
:
#ifndef USE_HEADERS
import Hello;
#else
#include "hello.cxxm"
#endif
int main() {
hello();
float x[4];
[[maybe_unused]] auto v = vload128(x);
return 0;
}
Compiling with headers:
$ clang++ -std=c++23 use.cpp -DUSE_HEADERS -o Hello.
out
$ ./Hello.out
Hello World!
Compiling with modules:
$ clang++ -std=c++23 --precompile hello.cxxm -o M-he
llo.pcm
$ clang++ -std=c++23 use.cpp -fmodule-file=Hello=M-hell
o.pcm M-hello.pcm -o Hello_mod.out
In file included from use.cpp:2:
/home/chriselrod/Documents/progwork/cxx/experiments/modules/hello.cxxm:16:14: error: no matching function for call to '_mm_loadu_ps'
16 | return _mm_loadu_ps(p);
| ^~~~~~~~~~~~
use.cpp:10:29: note: in instantiation of function template specialization 'vload128<float>' requested here
10 | [[maybe_unused]] auto v = vload128(x);
| ^
1 error generated.
using something like
template <typename T> auto vload128(const T *p) {
if constexpr (std::same_as<T, float>) {
const float *fp = p;
return _mm_loadu_ps(fp);
} else if constexpr (std::same_as<T, double>) {
const double *dp = p;
return _mm_loadu_pd(dp);
} else {
const __m128i *ip = reinterpret_cast<const __m128i *>(p);
return _mm_loadu_si128(ip);
}
}
instead allows it to compile, even though fp
and dp
should have the same type as p
.
Another workaround is to declare explicit instantiations of the template within the module that defines it.
I can't reproduce this in my local environment with trunk (0182f5174f7cab31f8275718ae0ac).
My standard library is libstdc++ 10.2 (this may not be relevent) in linux.
Can you try again with trunk?
I just stumbled across this bug as well, upgrading from a clang trunk from around end of June to this commit: https://github.com/ChuanqiXu9/clangd-for-modules/commit/e58317686e96feb5297157cc17dddd47722ec20c. I'm also using <boost/unordered/unordered_flat_map.hpp>
in my code.
I managed to bisect it to 91d40ef6e369a73b0147d9153a95c3bc63e14102 . Maybe something in that commit inadvertently touched some code related to name lookup or module visibility?
It is a bit weird though that the date of that commit is after the report date of this bug...
I managed to work around it for now by re-applying 91d40ef6e369a73b0147d9153a95c3bc63e14102 on top of https://github.com/ChuanqiXu9/clangd-for-modules/commit/e58317686e96feb5297157cc17dddd47722ec20c . I guess reapplying it on main
should also work.
After reapplying https://github.com/llvm/llvm-project/commit/91d40ef6e369a73b0147d9153a95c3bc63e14102 , the reproducer from here now fails again, but luckily my code doesn't seem to trigger that case.
@jiixyj I've relanded https://github.com/llvm/llvm-project/pull/102287. Can you verify if this is still a problem?
@ChuanqiXu9 : Sadly, with 3c9e3457d7e3153f449bef047d4deb297126f446 I still get the errors when including <boost/unordered/unordered_flat_map.hpp>
:
/home/jan/.conan2/p/b/boost77375cc29ad40/p/include/boost/unordered/detail/foa/core.hpp:291:12: error: no matching function for call to '_mm_movemask_epi8'
291 | return _mm_movemask_epi8(
| ^~~~~~~~~~~~~~~~~
and
/home/jan/.conan2/p/b/boost77375cc29ad40/p/include/boost/unordered/detail/foa/core.hpp:1070:36: error: no matching function for call to 'size_index_for'
1070 | auto groups_size_index=size_index_for<group_type,size_policy>(n);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...and some more similar ones.
I looked at the differences between the original PR and the relanded version. It turns out if I make Decl::isInAnotherModuleUnit()
look like the following, the errors go away:
bool Decl::isInAnotherModuleUnit() const {
auto *M = getOwningModule();
#if 1
if (!M || !M->isNamedModule())
return false;
#else
if (!M)
return false;
// FIXME or NOTE: maybe we need to be clear about the semantics
// of clang header modules. e.g., if this lives in a clang header
// module included by the current unit, should we return false
// here?
//
// This is clear for header units as the specification says the
// header units live in a synthesised translation unit. So we
// can return false here.
M = M->getTopLevelModule();
if (!M->isNamedModule())
return false;
#endif
return M != getASTContext().getCurrentNamedModule();
}
The code between #else
and #endif
is the current code in the main
branch.
This is just a shot in the dark as I'm really not familiar with this code, but maybe there should be a check for the global module again? Like this:
bool Decl::isInAnotherModuleUnit() const {
auto *M = getOwningModule();
if (!M)
return false;
+ if (M->isGlobalModule())
+ return false;
+
// FIXME or NOTE: maybe we need to be clear about the semantics
// of clang header modules. e.g., if this lives in a clang header
// module included by the current unit, should we return false
// here?
//
// This is clear for header units as the specification says the
// header units live in a synthesised translation unit. So we
// can return false here.
M = M->getTopLevelModule();
if (!M->isNamedModule())
return false;
return M != getASTContext().getCurrentNamedModule();
}
Otherwise, getTopLevelModule()
turns e.g. my.module.<global>
into my.module
.
Also I'm wondering if any change in isInAnotherModuleUnit()
should be mirrored in isInCurrentModuleUnit()
?
@jiixyj Thanks for the analysis, it is helpful. But the suggested check may not be the case. Since the GMF may be in the current module unit semantically. I'll try if the minimal reproducer from @chriselrod works. If yes I guess I can fix it quickly. But if not, maybe it will be better to get a reproducer from you.
Also I'm wondering if any change in isInAnotherModuleUnit() should be mirrored in isInCurrentModuleUnit()?
Maybe not. Since a translation unit may not be a module unit. Otherwise we can implement isInAnotherModuleUnit()
as !isInCurrentModuleUnit()
Here is a minimal reproducer. I used cvise to create this, plus some simplifications by hand.
map.cppm:
module;
static void fun(long);
template <typename = void> struct a {
a() { fun(load()); }
long load();
};
export module map;
export using map = a<>;
map.cpp:
import map;
map m;
Run it like:
$ clang++ -std=c++23 --precompile map.cppm -o M-map.pcm && clang++ -std=c++23 -c map.cpp -fmodule-file=map=M-map.pcm
In file included from map.cpp:1:
map.cppm:6:9: error: no matching function for call to 'fun'
6 | a() { fun(load()); }
| ^~~
map.cpp:2:5: note: in instantiation of member function 'a<>::a' requested here
2 | map m;
| ^
1 error generated.
For the record, here is the resulting map.cppm
from cvise before my manual simplifications:
...and here the "start" map.cppm
provided as input to cvise (before macro expansion with -save-temps
):
I remember calling the intrinsics a red herring somewhere, as I later learned it seems to actually be about static
, as in your minimal example.
Out of curiosity, have you also gotten a lot of linker errors that only show up when using modules, not when using headers?
So if I understand correctly, _mm_movemask_epi8
is considered "TU-local" according to https://eel.is/c++draft/basic.link#15. Therefore, "exposing" it from the module (via inline
or a function template) is ill-formed.
This is the definition of _mm_movemask_epi8
from Clang 18 (/usr/lib/clang/18/include/emmintrin.h
):
static __inline__ int __DEFAULT_FN_ATTRS _mm_movemask_epi8(__m128i __a) {
return __builtin_ia32_pmovmskb128((__v16qi)__a);
}
__DEFAULT_FN_ATTRS
defines the __always_inline__
attribute among some others.
Because this is static inline, this becomes "TU-local" when used in a module unit. In the traditional C++ compilation model this is no problem, as each translation unit gets its own copy of the static inline method.
Interestingly, GCC defines their _mm_*
intrinsics without static
:
extern __inline int __attribute__((__gnu_inline__, __always_inline__, __artificial__))
_mm_movemask_epi8 (__m128i __A)
{
return __builtin_ia32_pmovmskb128 ((__v16qi)__A);
}
I'm not sure what the exact semantics of extern
in combination with __inline
(with a leading double underscore) and __always_inline__
are, but at least it would not be TU-local I guess.
Maybe Clang could define their intrinsics to be more "modules friendly", like GCC? Or, maybe Clang could just "make modules work" as an exception for ___always_inline__
_ static
inline
functions -- the __always_inline__
means the function should "disappear" anyways.
Or could static inline
in the GMF be made to "just work" in general? static inline
is a common idiom especially in C when you want to have code in headers without any ODR issues.
On the other hand, the size_index_for
error seems to be a "bug" in boost::unordered. It is defined like this:
template<typename Group,typename SizePolicy>
static inline std::size_t size_index_for(std::size_t n)
{
/* n/N+1 == ceil((n+1)/N) (extra +1 for the sentinel) */
return SizePolicy::size_index(n/Group::N+1);
}
It should probably be just inline
, not static inline
. Again, in the old translation model this wouldn't make any difference, but with modules it does (IIUC).
Out of curiosity, have you also gotten a lot of linker errors that only show up when using modules, not when using headers?
Yes, I also got linker errors here and there when trying to debug my issue with boost's unordered_map when including it in a module unit. This is just a guess, but maybe Clang gets confused with all those static inline
functions floating around combined with being in a "modules context". Maybe things like reachability (examples: https://eel.is/c++draft/module.global.frag#6) enter the picture as well... All compiles "successfully" (even though it is actually ill-formed according to the standard), but then it fails at the linking stage because the code for some of those functions is not emitted?
In https://github.com/llvm/llvm-project/issues/78173 @ChuanqiXu9 also mentioned that Clang's diagnostics when static
/static inline
is mixed with modules are not the best currently.
I can fix the compiler errors if I replace static
with extern
for _mm_movemask_epi8
and _mm_cmpeq_epi8
in "llvm-prefix/lib/clang/20/include/emmintrin.h", and remove the static
from boost::unordered's size_index_for
.
There is an interesting discussion thread from when GCC decided to convert all their static inline
SIMD intrinsics to extern inline
: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=34000
It appears that using static inline
functions inside inline
functions is problematic even in C, so this class of issue is not just limited to C++20 modules.
edit: Another interesting discussion about static
vs. extern
for SIMD intrinsics: https://stackoverflow.com/questions/57105290/static-vs-external-intrinsics The accepted answer is from a GCC developer.
What do you think about something like the following patch to Sema::AddOverloadCandidate
? The intention is to have a special case for static inline
functions that are defined in the GMF and don't make them invisible to name lookup. This should hopefully be minimally invasive as it just handles the common idiom of static inline
functions from (C) headers.
I'm not sure if this is blessed by the standard. But maybe it's OK as the GMF is intended as a compatibility mechanism for headers.
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index fd88b6a74297..103eec3c371f 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -6872,21 +6872,33 @@ void Sema::AddOverloadCandidate(
// Functions with internal linkage are only viable in the same module unit.
if (getLangOpts().CPlusPlusModules && Function->isInAnotherModuleUnit()) {
/// FIXME: Currently, the semantics of linkage in clang is slightly
/// different from the semantics in C++ spec. In C++ spec, only names
/// have linkage. So that all entities of the same should share one
/// linkage. But in clang, different entities of the same could have
/// different linkage.
- NamedDecl *ND = Function;
- if (auto *SpecInfo = Function->getTemplateSpecializationInfo())
+ NamedDecl *ND;
+ bool IsInlineFunctionInGMF;
+ if (auto *SpecInfo = Function->getTemplateSpecializationInfo()) {
ND = SpecInfo->getTemplate();
+ IsInlineFunctionInGMF = false;
+ } else {
+ ND = Function;
+ /// As a special case, don't remove a "static inline" function declared
+ /// in the GMF from the overload set since this is a common pattern in C
+ /// code.
+ IsInlineFunctionInGMF =
+ Function->getOwningModule() &&
+ Function->getOwningModule()->isGlobalModule() &&
+ Function->isInlineSpecified();
+ }
- if (ND->getFormalLinkage() == Linkage::Internal) {
+ if (ND->getFormalLinkage() == Linkage::Internal && !IsInlineFunctionInGMF) {
Candidate.Viable = false;
Candidate.FailureKind = ovl_fail_module_mismatched;
return;
}
}
if (isNonViableMultiVersionOverload(Function)) {
Candidate.Viable = false;
@jiixyj as you mentioned, this is due to the limitations of TU locals
from the standard. Or in another word, the limitation was introduced by the nature of static entities. I feel the proposed suggestion is somewhat hack. I guess the best solution here is to mark all the intrinsics as inline.
I agree, this is more of a hack. I do wonder though why the error only appears when the call to the static inline
function is dependent on a template parameter for the error to appear. Here is a simpler reproducer:
"map.cppm":
module;
static inline void fun(long) {}
template <typename T = long> void a() { fun(T{}); }
export module map;
export using ::a;
"map.cpp":
import map;
auto m = (a(), 0);
If I either:
a()
a non-template functionfun()
qualified like this: ::fun()
...the problem goes away.
I guess it is like this because dependent names are looked up at the point of template instantiation -- and at that point, the static inline
function is "outside" the current module so it is not considered for name lookup (?). I wonder why the error also goes away when I qualify ::fun()
, though. Is ADL involved as well?
I wonder why the error also goes away when I qualify ::fun(), though. Is ADL involved as well?
Yeah, I think so.
I guess it is like this because dependent names are looked up at the point of template instantiation -- and at that point, the static inline function is "outside" the current module so it is not considered for name lookup (?).
Yeah, according to the codes you cited, it may be the case.
"map.cppm":
module; static inline void fun(long) {} template <typename T = long> void a() { fun(T{}); } export module map; export using ::a;
"map.cpp":
import map; auto m = (a(), 0);
For this reproducer, my feeling is, if we implement the standard strictly, the compiler may not have a chance to look at map.cpp
, it will emit errors when compiling map.cppm
.
But this is the reason why I didn't implement the check for such a long time. We have too many static functions in headers in this ecosystem. I fear it will make a lot of things gets broken.
Now I changed my mind slightly. I think you can submit your change as a hack workaround due to the current C++ ecosystem.
And it is not conflicting with the change we need to made for intrinsics and give warnings for such leaked TU-locals.
Thank you for your feedback! I'll try to create a PR and to write some tests.
It looks like this was discussed in the committee before: https://github.com/cplusplus/nbballot/issues/427 There was one paper, but it wasn't accepted. Still, the committee recognizes the problem and they encourage further work on this.
And in Clang it actually works most of the time! In MSVC as well as far as I can see. But finding wording for this that can go into the standard is a hard problem it looks like.
edit: correction: MSVC fails this:
module;
static inline int fun() { return 0; }
template <typename G = void> int a() { return fun(); }
export module map;
export using ::a;
import map;
auto _ = (a(), 0);
...with:
map.cppm(3): error C2129: static function 'int fun(void)' declared but not defined
map.cppm(3): note: see declaration of 'fun'
It looks like this was discussed in the committee before: cplusplus/nbballot#427 There was one paper, but it wasn't accepted. Still, the committee recognizes the problem and they encourage further work on this.
This is another topic. It is about header units not the named modules. As far as I remember, I didn't see such disucssion in commitee.
I opened a PR: https://github.com/llvm/llvm-project/pull/104701
I expanded the fix/hack for implicit instantiations of templates with internal linkage, as people write code like that: https://github.com/boostorg/unordered/blob/a39cf60e93ab7ee1782bae4ced211dc9f6eff751/include/boost/unordered/detail/foa/core.hpp#L855
With this change, my usage of Boost's unordered_flap_{map,set}
now works without errors.
That is, code using
immintrin.h
tends to fail to compile when using modules while working fine with headers. I'll try to produce a minimal example in the next few hours. For now, I have an example using boost_unordered. When problems showed up in my own code using intrinsics, I could generally fix it by declaring all arguments as variables, and then passing the lvalues to the intriinsic function.Hello.cxxm:
user.cpp:
Compiling with headers:
With modules:
results in
I could file this as a
boost_unordered
issue or make a PR there, as I've generally found I can work around the problem. But I'll see about creating a minimal reproducer using#include <immintrin.h>
directlry that works with headers but fails with modules.