Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use [[no_unique_address]] after Clang supports it on Windows #1364

Open
1 of 3 tasks
StephanTLavavej opened this issue Oct 11, 2020 · 14 comments
Open
1 of 3 tasks

Use [[no_unique_address]] after Clang supports it on Windows #1364

StephanTLavavej opened this issue Oct 11, 2020 · 14 comments
Labels
compiler Compiler work involved performance Must go faster vNext Breaks binary compatibility

Comments

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 11, 2020

VS 2019 16.9 will support [[no_unique_address]]. Clang lists this as supported in Clang 9. However, we suspect that it is not yet supported when Clang targets Windows, because MSVC didn't support it. So, #1363 is commenting out all of our [[no_unique_address]] usage.

  • Confirm whether Clang supports [[no_unique_address]] on Windows. (Clang does not.)
  • If it does, proceed to the next step, otherwise we should file an LLVM bug requesting such support, then wait for it to be implemented and available in the version of Clang that ships with VS.
  • Finally, we should begin using [[no_unique_address]] for C++20-only code.
@StephanTLavavej StephanTLavavej added performance Must go faster compiler Compiler work involved labels Oct 11, 2020
@CaseyCarter
Copy link
Contributor

VS 2019 16.9 will support [[no_unique_address]]. Clang lists this as supported in Clang 9. However, we suspect that it is not yet supported when Clang targets Windows, because MSVC didn't support it. So, #1363 is commenting out all of our [[no_unique_address]] usage.

  • Confirm whether Clang supports [[no_unique_address]] on Windows.

When targeting MSABI Clang emits unknown attribute warnings for [[no_unique_address]]. This is peculiar given that the exact same binary with a different --target triple may understand the attribute just fine. I assume it was the simplest way to have the attribute do nothing until MSVC implements behavior for clang to mimic ensuring ABI compatibility.

  • If it does, proceed to the next step, otherwise we should file an LLVM bug requesting such support, then wait for it to be implemented and available in the version of Clang that ships with VS.

I would guess that work on this will be blocked on access to an MSVC that implements [[no_unique_address]] for validation.

@AlexGuteniev
Copy link
Contributor

I recall first reinventing and then reusing existent compressed pair for barrier callback.
Just a reminder that at least C++20 uses of compressed pairs can be eliminated after this arrives.

@StephanTLavavej StephanTLavavej added the vNext Breaks binary compatibility label May 21, 2021
@StephanTLavavej StephanTLavavej added vNext Breaks binary compatibility and removed vNext Breaks binary compatibility labels Aug 19, 2021
@StephanTLavavej
Copy link
Member Author

MSVC has implemented [[msvc::no_unique_address]] which is unconditionally available. However, it's probably too late to request Clang support and take advantage of it before the C++20 ABI lockdown of Ranges/Format/Chronat happens. We may be able to use this for C++23 before vNext though.

@AlexGuteniev
Copy link
Contributor

@StephanTLavavej can plain [[no_unique_address]] be used in Clang, which is also unconditional?

struct S1 {};
struct S2 {};


#if defined(_MSC_VER)
#pragma warning(disable:4848)
#define _NO_UNIQUE_ADDRESS [[msvc::no_unique_address]]
#else
#define _NO_UNIQUE_ADDRESS [[no_unique_address]]
#endif

struct S
{
    _NO_UNIQUE_ADDRESS S1 s1;
    S2 s2;
};

int s()
{
    return sizeof(S);
}

https://godbolt.org/z/9PbKxM8ab

@AlexGuteniev
Copy link
Contributor

@fsb4000 obseved that on Windows Clang does not support [[no_unique_address]] https://godbolt.org/z/x1bjbGz7a

@jyknight
Copy link

FYI, https://reviews.llvm.org/D110485 to support no_unique_address for Windows targets -- which, indeed, is blocked on concern of ABI incompatibility with MSVC.

I can't tell if there's some other outcome (besides Clang continuing to ignore it on windows for now) which would be desirable -- if so, might be nice to chime in!

@mstorsjo
Copy link

mstorsjo commented Feb 9, 2022

Based on observing various MSVC versions, while newer versions of MSVC (since 16.9) do support [[no_unique_address]], it seems to me like it has no effect, while [[msvc::no_unique_address]] does have effect - is that the correct conclusion? https://godbolt.org/z/xjec9vr7M shows what I'm observing.

@CaseyCarter
Copy link
Contributor

Based on observing various MSVC versions, while newer versions of MSVC (since 16.9) do support [[no_unique_address]], it seems to me like it has no effect, while [[msvc::no_unique_address]] does have effect - is that the correct conclusion? https://godbolt.org/z/xjec9vr7M shows what I'm observing.

That is precisely the intended behavior. We were concerned about interjecting [[no_unique_address]] as an attribute that affects ABI while claiming ABI compatibility from VS 2015 to VS 2022, doubly so after we shipped a couple of point releases of VS 2019 that silently accepted [[no_unique_address]] and did nothing with it. [[msvc::no_unique_address]] is a compromise, in that we never shipped a compiler that wouldn't either diagnose or implement the ABI-affecting behavior.

@mstorsjo
Copy link

mstorsjo commented Feb 9, 2022

Based on observing various MSVC versions, while newer versions of MSVC (since 16.9) do support [[no_unique_address]], it seems to me like it has no effect, while [[msvc::no_unique_address]] does have effect - is that the correct conclusion? https://godbolt.org/z/xjec9vr7M shows what I'm observing.

That is precisely the intended behavior. We were concerned about interjecting [[no_unique_address]] as an attribute that affects ABI while claiming ABI compatibility from VS 2015 to VS 2022, doubly so after we shipped a couple of point releases of VS 2019 that silently accepted [[no_unique_address]] and did nothing with it. [[msvc::no_unique_address]] is a compromise, in that we never shipped a compiler that wouldn't either diagnose or implement the ABI-affecting behavior.

Thanks for the clarification!

@AlexGuteniev
Copy link
Contributor

optional and expected currently don't benefit from compressed pair. They should use [[no_unique_address]] I guess.

@frederick-vs-ja
Copy link
Contributor

If VS can be updated to use Clang 18 before C++23 ABI lockdown, it seems possible to use [[msvc::no_unique_address]] for in_value_result, out_value_result, and expected.

@frederick-vs-ja
Copy link
Contributor

If VS can be updated to use Clang 18 before C++23 ABI lockdown, it seems possible to use [[msvc::no_unique_address]] for in_value_result, out_value_result, and expected.

Seems doable now. Although it's somehow inconsistent that EDG recognizes [[no_unique_address]] but not [[msvc::no_unique_address]] in VS IntelliSense (DevCom-1485501).

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Sep 16, 2024

Oops. Something is wrong for empty bases (Godbolt link), which is critical for views which use CRTP.

template<class D>
struct CrtpBase {};

struct Empty {};

struct X : CrtpBase<X> {
    int n;
    [[msvc::no_unique_address]] Empty e;
};

static_assert(sizeof(X) == sizeof(int)); // OK with Clang, failed with MSVC

struct Y : CrtpBase<Y> {
    [[msvc::no_unique_address]] Empty e;
    int n;
};

static_assert(sizeof(Y) == sizeof(int)); // Both OK

Is this DevCom-10588147?

@CaseyCarter
Copy link
Contributor

Is this DevCom-10588147?

Yes it is. The fix should ship in VS 2022 17.12p3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler Compiler work involved performance Must go faster vNext Breaks binary compatibility
Projects
None yet
Development

No branches or pull requests

6 participants