[C++20] [Modules] Merging of lambda types

[ilya-biryukov]

I believe this code should not produce the static assertion failure:

//--- lambda.h
#pragma once

inline auto cmp = [](auto l, auto r) {
  return l < r;
};
//--- A.cppm
module;
#include "lambda.h"

export module A;
export auto c1 = cmp;
export using ::cmp;
//--- B.cppm
module;
#include "lambda.h"

export module B;
export auto c2 = cmp;
export using ::cmp;
//--- use.cppm
module;

export module use;

import A;
import B;

static_assert(__is_same(decltype(c1), decltype(c2))); // should succeed.
auto x = cmp; // cmp must not be ambiguous,

// Build with:
// clang++ -std=c++20 --precompile -o A.pcm A.cppm
// clang++ -std=c++20 --precompile -o B.pcm B.cppm
// clang++ -std=c++20 -fprebuilt-module-path=. use.cppm

As the lambda meets all requirements specified in basic.def.odr and cmp is declared inline. I.e. this should be no different from explicit class declaration from lambda.

However, the static assertion currently fails and the use of 'cmp' produces an "ambiguous reference" error.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[llvmbot]

@llvm/issue-subscribers-c-20

[llvmbot]

@llvm/issue-subscribers-clang-modules

[ChuanqiXu9]

Confirmed. First I hesitated since I know each lambda has a unique closure type. But later I found https://eel.is/c++draft/basic.def.odr#14.4:

Each such definition shall consist of the same sequence of tokens, where the definition of a closure type is considered to consist of the sequence of tokens of the corresponding lambda-expression.

And https://eel.is/c++draft/basic.def.odr#14.6

In each such definition, except within the default arguments and default template arguments of D, corresponding lambda-expressions shall have the same closure type (see below).

So static_assert shouldn't fail.

Also I think the inline part may be redundant, no matter it is inline or not, the declaration is attached to global module fragment.

[aaronmondal]

I believe this is implicitly causing libcxx to fail when we include anything that makes use of __compare/synth_three_way.h in GMFs and the locations of the headers differ during compilation and precompilation. For instance if we include <compare> several times in the interface and/or implementation GMFs, we get something like this:

error: redeclaration of '__synth_three_way' with a different type:
    'const std::__1::(lambda at <path 1>/libcxx/include/__compare/synth_three_way.h:29:3)'
 vs 'const std::__1::(lambda at <path 2>/libcxx/include/__compare/synth_three_way.h:29:3)'

This patch seems to fix the issue, but I'm not sure what the implications of removing this lambda may be:

--git a/libcxx/include/__compare/synth_three_way.h b/libcxx/include/__compare/synth_three_way.h
index fa8cbda79ba2..a6df1b936c72 100644
--- a/libcxx/include/__compare/synth_three_way.h
+++ b/libcxx/include/__compare/synth_three_way.h
@@ -25,8 +25,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 
 // [expos.only.func]
 
-_LIBCPP_HIDE_FROM_ABI inline constexpr auto __synth_three_way =
-  []<class _Tp, class _Up>(const _Tp& __t, const _Up& __u)
+template<class _Tp, class _Up>
+_LIBCPP_HIDE_FROM_ABI inline constexpr auto __synth_three_way(const _Tp& __t, const _Up& __u)
     requires requires {
       { __t < __u } -> __boolean_testable;
       { __u < __t } -> __boolean_testable;
@@ -42,7 +42,7 @@ _LIBCPP_HIDE_FROM_ABI inline constexpr auto __synth_three_way =
   };
 
 template <class _Tp, class _Up = _Tp>
-using __synth_three_way_result = decltype(std::__synth_three_way(declval<_Tp&>(), declval<_Up&>()));
+using __synth_three_way_result = decltype(_VSTD::__synth_three_way(declval<_Tp&>(), declval<_Up&>()));
 
 #endif // _LIBCPP_STD_VER > 17

@mumbleskates Could you explain why this lambda is neccessary?

Apart from having to add a template deduction guide for back_insert_iterator this is the only blocker I found so far for using libcxx freely in global module fragments.

I initially tried only changing (in Line 29) inline constexpr to static inline constexpr, but then the using __synth_three_way_result-line raises this buggy error message which I think was already reported in some other module-related issue:

error: type alias template redefinition with different types
    ('decltype(__synth_three_way(declval<_Tp &>(), declval<_Up &>()))'
  vs 'decltype(__synth_three_way(declval<_Tp &>(), declval<_Up &>()))')

[mumbleskates]

@aaronmondal the lambda is transcribed pretty much verbatim from the draft standard. It was written here, and there was some discussion about whether or not to make it a lambda (cc @ldionne). we're supposed to have ODR protection with _LIBCPP_HIDE_FROM_ABI though i'm not familiar with the different situations that may be arising here.

it looks like arthur made the change e0e7bd1 in march this year updating the __synth_three_way_result definition to use the explicit namespace, which may have been good to have in there originally but also should have used _VSTD. it does not look like this commit went through review, and he's gone now.

@aaronmondal if you make only the latter change, to __synth_three_way_result to use _VSTD::, does that alone fix it?

[aaronmondal]

@mumbleskates Thanks for taking a look at this!

the lambda is transcribed pretty much verbatim from the draft standard

Ah ok found it: https://eel.is/c++draft/expos.only.func#conceptref:three_way_comparable_with

if you make only the latter change, to __synth_three_way_result to use _VSTD::, does that alone fix it?

Only changing this still raises the redeclaration of '__synth_three_way' with a different type error.

I think this is a bug with modules, not with libcxx. However, if the simpler non-lambda variant satisfies all tests (haven't checked yet), I think it may actually desirable to use that variant. From the linked discussion it seems to me that the lambda was more or less an arbitrary choice for the sake of sticking close to what the exposition in the standard looks like. I think the original argument had something to do with not having to use _VSTD, but since _VSTD should be used now anyways the non-lambda variant seems a lot easier to comprehend to me.

Fixing the lambda resolution in modules seems very hard in comparison to changing 3 lines in libcxx. I checked the cppcoreguidelines for stylistic reasons on why one may want to use a lambda here but couldn't find any.

So for the sake of readability and module compatibility (even if technically just a workaround) I think it would be nice to change this.

I'll send the patch for review 😊

[mumbleskates]

this really does sound a lot like it is surfacing a bug in modules. IIRC clang's modules are not to spec so if they're causing a problem here, because this lambda should be fully guarded against resolving a different definition in different TUs, that's something that should be fixed rather than papered over.

[mumbleskates]

i'm in favor of fixing the __synth_three_way_result to use _VSTD:: but i'm much less sure about the other part... not because i have strong feelings about how __synth_three_way should be specifically, but because of the underlying motivation for the change.

[ChuanqiXu9]

@aaronmondal hi, sorry for I may not be able to look into details soon. But I am interested in your use cases for C++20 Modules. Since it inspires me that the C++20 Modules have more users. May I ask the use case or the project?

[aaronmondal]

@ChuanqiXu9

Our use case in general is that we want to build heterogeneous applications with the smallest code footprint possible while still targeting as many platforms as possible. To do this we are trying to create a toolchain that natively supports SYCL, a more-or-less unified wrapper around frameworks like CUDA, HIP and OneAPI. We want to offload all the complexity of setting up the different heterogeneous toolchains, frameworks, dependencies etc to the build system so that developers "just press one button" and have all these things set up automatically (similarly to an IDE). In our team we've had a very good experience with this approach so far, because this one-button-press allows us to set up (and tear down) our entire development environment with a single command (or no command at all since builds will automatically set up the environment if it is not present yet). Since we encapsulate the entire toolchain and build every dependency from source we also know for sure that everyone is using the exact same compiler, the exact same tools etc.. This way we don't really need to think about toolchain compatibilities (and are able to use highly unstable upstream versions of LLVM as our default compiler for everything 😈)

Our approach worked so well for us that we decided to make end users build our software themselves instead of distributing binaries. So we will try to distribute our software as source code (including the toolchain) instead of distributing binaries. For this to be viable, the build process needs to be as easy and as convenient as a regular binary software installation. When end users are expected to build the software locally on their machines for their specific hardware, compile time is no longer just an inconvenience for developers - it becomes a UX issue for end users. If we are able to make an installation process just 10% faster I see it as a big win. Considering that modules could improve compile times significantly more than that, they are incredibly relevant for our end-user experience.

While the current implementations of C++ Modules do not yet support heterogeneous code, targets like NVPTX and AMDGPU will also be an incredibly interesting use case. For instance, SYCL for NVPTX is more or less a header-wrapper around the CUDA headers. Heterogeneous compilations are probably the most time consuming compilations out of all the build targets we currently have. If it were possible to leverage modules in e.g. a CUDA header unit, the compile time improvements could be very significant. As a broader use case, if AI frameworks like TensorFlow and PyTorch (which heavily rely on GPU kernels) were to take up 10% less cloud build minutes during CI runs I think that would also be a big win.

Apart from the compile-time improvements, we've also found the module syntax to be a lot nicer to work with compared to header inclusions. Even without all the above mentioned practical benefits, I personally would probably still use modules just for the improved code aesthetics and the prettier build graphs that the module system encourages 😄

The initial project that will use our toolchain will be a Cryptominer. The code for that is not yet open source. I'll ping you as soon as we release this project that will use C++20 modules. In fact, we only built all the toolchain-related things because it was effectively impossible to collaborate on a project where every contributor needs to set up several toolchains spread over more than 10 different, partially codependent repositories just to get a simple build to work.

[ChuanqiXu9]

@ChuanqiXu9

Our use case in general is that we want to build heterogeneous applications with the smallest code footprint possible while still targeting as many platforms as possible. To do this we are trying to create a toolchain that natively supports SYCL, a more-or-less unified wrapper around frameworks like CUDA, HIP and OneAPI. We want to offload all the complexity of setting up the different heterogeneous toolchains, frameworks, dependencies etc to the build system so that developers "just press one button" and have all these things set up automatically (similarly to an IDE). In our team we've had a very good experience with this approach so far, because this one-button-press allows us to set up (and tear down) our entire development environment with a single command (or no command at all since builds will automatically set up the environment if it is not present yet). Since we encapsulate the entire toolchain and build every dependency from source we also know for sure that everyone is using the exact same compiler, the exact same tools etc.. This way we don't really need to think about toolchain compatibilities (and are able to use highly unstable upstream versions of LLVM as our default compiler for everything 😈)

Our approach worked so well for us that we decided to make end users build our software themselves instead of distributing binaries. So we will try to distribute our software as source code (including the toolchain) instead of distributing binaries. For this to be viable, the build process needs to be as easy and as convenient as a regular binary software installation. When end users are expected to build the software locally on their machines for their specific hardware, compile time is no longer just an inconvenience for developers - it becomes a UX issue for end users. If we are able to make an installation process just 10% faster I see it as a big win. Considering that modules could improve compile times significantly more than that, they are incredibly relevant for our end-user experience.

While the current implementations of C++ Modules do not yet support heterogeneous code, targets like NVPTX and AMDGPU will also be an incredibly interesting use case. For instance, SYCL for NVPTX is more or less a header-wrapper around the CUDA headers. Heterogeneous compilations are probably the most time consuming compilations out of all the build targets we currently have. If it were possible to leverage modules in e.g. a CUDA header unit, the compile time improvements could be very significant. As a broader use case, if AI frameworks like TensorFlow and PyTorch (which heavily rely on GPU kernels) were to take up 10% less cloud build minutes during CI runs I think that would also be a big win.

Apart from the compile-time improvements, we've also found the module syntax to be a lot nicer to work with compared to header inclusions. Even without all the above mentioned practical benefits, I personally would probably still use modules just for the improved code aesthetics and the prettier build graphs that the module system encourages 😄

The initial project that will use our toolchain will be a Cryptominer. The code for that is not yet open source. I'll ping you as soon as we release this project that will use C++20 modules. In fact, we only built all the toolchain-related things because it was effectively impossible to collaborate on a project where every contributor needs to set up several toolchains spread over more than 10 different, partially codependent repositories just to get a simple build to work.

Oh, I see. Thanks for the very detailed introduction! I love it. I'll be appreciated being pinged about modules related things. Thanks

[ChuanqiXu9]

I sent https://reviews.llvm.org/D140002. But the original reproducer can't pass still due to a separate bug: #59513. I changed it a little bit to address it.

Note that this only address the following two cases:

import A;
import B;

#include "lambda.h"
import A;

But it can't handle the following case:

import A;
#include "lambda.h"

We need to handle the case in the Sema side. I plan to file another issue for this after we land that one.