Page MenuHomePhabricator

[libcxx][NFC] utilises compiler builtins for unary transform type-traits
ClosedPublic

Authored by cjdb on Aug 11 2022, 3:26 PM.

Diff Detail

Event Timeline

cjdb created this revision.Aug 11 2022, 3:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 3:26 PM
cjdb requested review of this revision.Aug 11 2022, 3:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 3:26 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik requested changes to this revision.Aug 11 2022, 3:32 PM
philnik added a subscriber: philnik.

Which compiler supports these builtins? Neither GCC nor clang accept __add_lvalue_reference or __add_pointer: https://godbolt.org/z/nvxqWTGY1

This revision now requires changes to proceed.Aug 11 2022, 3:32 PM
cjdb added a comment.Aug 11 2022, 5:16 PM

Which compiler supports these builtins? Neither GCC nor clang accept __add_lvalue_reference or __add_pointer: https://godbolt.org/z/nvxqWTGY1

See D116203.

Which compiler supports these builtins? Neither GCC nor clang accept __add_lvalue_reference or __add_pointer: https://godbolt.org/z/nvxqWTGY1

See D116203.

Could you rebase on top of D116203 to run the clang patch with this patch together in the CI? I think it would be good to land these two patches at the same time.
Do you have any benchmarks to show how much of a difference it makes?

libcxx/include/__type_traits/add_lvalue_reference.h
21–30

For all of the changes: Please clang-format them and use using instead of typedef.

libcxx/include/__type_traits/decay.h
64

Please also add a comment to other longer preprocessor conditions.

libcxx/include/__type_traits/is_referenceable.h
23

I think a better name would be __libcpp_is_referenceable. __referenceable sounds a lot like a concept, not a type trait.

libcxx/include/__type_traits/remove_cvref.h
30

This is just a duplicate of line 43. Could you move them out of the #if __has_builtin() instead?

cjdb updated this revision to Diff 452261.Aug 12 2022, 11:48 AM
cjdb marked 4 inline comments as done.
cjdb edited the summary of this revision. (Show Details)

responds to feedback, updates commit message so it pulls in D116203 when CI is on

cjdb updated this revision to Diff 452288.Aug 12 2022, 1:08 PM

made a change to a diagnostic in D116203, so rebasing here

philnik accepted this revision.Aug 14 2022, 5:58 AM

LGTM. You can ignore the test timeout.

This revision is now accepted and ready to land.Aug 14 2022, 5:58 AM
This revision was landed with ongoing or failed builds.Aug 14 2022, 10:34 AM
This revision was automatically updated to reflect the committed changes.
cjdb reopened this revision.Aug 21 2022, 6:07 PM
This revision is now accepted and ready to land.Aug 21 2022, 6:07 PM
cjdb updated this revision to Diff 454351.Aug 21 2022, 6:08 PM

rebases to activate CI

This revision was landed with ongoing or failed builds.Aug 21 2022, 8:04 PM
This revision was automatically updated to reflect the committed changes.

The commit says NFC, but we found this to be an observable behavior change with __restrict now being dropped as part of std::decay: https://godbolt.org/z/zqvW478jq. It impacted us via use of std::make_pair which decays the types, and we had a static_assert checking that the type passed in had __restrict on it.

We have a workaround (just don't use std::make_pair), but I figured I should mention this since the commit is labelled NFC, so any observable change is a surprise.

IIUC, because __restrict is non-standard C++, it's implementation defined whether std::decay wants to modify it or not. Therefore, both before and after are "correct", and code should not rely on which option we choose.

cjdb added a comment.Aug 26 2022, 8:11 AM

The commit says NFC, but we found this to be an observable behavior change with __restrict now being dropped as part of std::decay: https://godbolt.org/z/zqvW478jq. It impacted us via use of std::make_pair which decays the types, and we had a static_assert checking that the type passed in had __restrict on it.

We have a workaround (just don't use std::make_pair), but I figured I should mention this since the commit is labelled NFC, so any observable change is a surprise.

IIUC, because __restrict is non-standard C++, it's implementation defined whether std::decay wants to modify it or not. Therefore, both before and after are "correct", and code should not rely on which option we choose.

Thanks for flagging this.

It was a deliberate choice to have __decay drop all CVR qualifiers in D116203, with the rationale being that if C++ supported restrict, it would probably be affected by std::decay. The libc++ change was supposed to be NFC, but I'd forgotten about the strictness of __decay by the time I'd gotten around to changing libc++ code.

I see two paths forward:

  1. Wait to see if any libc++ maintainers have opinions regarding __restrict. I don't think that they have up until now, since their CI would've caught it otherwise; though they might want to retroactively have opinions now that we know people have been negatively impacted by this.
  2. Immediately patch Clang so that __decay doesn't impact __restrict, since people have been negatively impacted by this change.

The commit says NFC, but we found this to be an observable behavior change with __restrict now being dropped as part of std::decay: https://godbolt.org/z/zqvW478jq. It impacted us via use of std::make_pair which decays the types, and we had a static_assert checking that the type passed in had __restrict on it.

We have a workaround (just don't use std::make_pair), but I figured I should mention this since the commit is labelled NFC, so any observable change is a surprise.

IIUC, because __restrict is non-standard C++, it's implementation defined whether std::decay wants to modify it or not. Therefore, both before and after are "correct", and code should not rely on which option we choose.

Thanks for flagging this.

It was a deliberate choice to have __decay drop all CVR qualifiers in D116203, with the rationale being that if C++ supported restrict, it would probably be affected by std::decay. The libc++ change was supposed to be NFC, but I'd forgotten about the strictness of __decay by the time I'd gotten around to changing libc++ code.

I see two paths forward:

  1. Wait to see if any libc++ maintainers have opinions regarding __restrict. I don't think that they have up until now, since their CI would've caught it otherwise; though they might want to retroactively have opinions now that we know people have been negatively impacted by this.
  2. Immediately patch Clang so that __decay doesn't impact __restrict, since people have been negatively impacted by this change.
#include <type_traits>

int main() {
  int* __restrict const volatile a = nullptr;
  auto v = a;
  static_assert(std::is_same_v<decltype(v), int*>);
}

Clang says that __restrict is dropped here, so I think it should also be dropped when using std::decay. @cjdb could you make a follow-up to also fix this for GCC and add tests?

cjdb added a subscriber: jwakely.EditedAug 26 2022, 8:37 AM

The commit says NFC, but we found this to be an observable behavior change with __restrict now being dropped as part of std::decay: https://godbolt.org/z/zqvW478jq. It impacted us via use of std::make_pair which decays the types, and we had a static_assert checking that the type passed in had __restrict on it.

We have a workaround (just don't use std::make_pair), but I figured I should mention this since the commit is labelled NFC, so any observable change is a surprise.

IIUC, because __restrict is non-standard C++, it's implementation defined whether std::decay wants to modify it or not. Therefore, both before and after are "correct", and code should not rely on which option we choose.

Thanks for flagging this.

It was a deliberate choice to have __decay drop all CVR qualifiers in D116203, with the rationale being that if C++ supported restrict, it would probably be affected by std::decay. The libc++ change was supposed to be NFC, but I'd forgotten about the strictness of __decay by the time I'd gotten around to changing libc++ code.

I see two paths forward:

  1. Wait to see if any libc++ maintainers have opinions regarding __restrict. I don't think that they have up until now, since their CI would've caught it otherwise; though they might want to retroactively have opinions now that we know people have been negatively impacted by this.
  2. Immediately patch Clang so that __decay doesn't impact __restrict, since people have been negatively impacted by this change.
#include <type_traits>

int main() {
  int* __restrict const volatile a = nullptr;
  auto v = a;
  static_assert(std::is_same_v<decltype(v), int*>);
}

Clang says that __restrict is dropped here, so I think it should also be dropped when using std::decay. @cjdb could you make a follow-up to also fix this for GCC and add tests?

I can add tests for libc++, but I'm not familiar with the GCC codebase. @jwakely, do you have any opinions on this particular change, and if you're of a similar mind, would you mind helping out on the libstdc++ side of things please?

@cjdb I meant that the other decay implementation should be fixed to be the same as the builtin. IOW the one that GCC uses, since it doesn't have the builtin. Sorry for the confusion.

cjdb added a comment.Aug 26 2022, 9:29 AM

@cjdb I meant that the other decay implementation should be fixed to be the same as the builtin. IOW the one that GCC uses, since it doesn't have the builtin. Sorry for the confusion.

Thanks for clarifying. After double checking the Clang test, I see that __decay strips all qualifiers: not just __restrict. For example, int* _Nonnull will become int*. There are a lots of these, and I'm not sure how we'd want to handle this without ironically impacting build times.