This is an archive of the discontinued LLVM Phabricator instance.

Add std::assume_aligned
AbandonedPublic

Authored by ldionne on Aug 30 2021, 1:06 AM.

Details

Reviewers
chandlerc
EricWF
gonzalobg
Group Reviewers
Restricted Project
Summary

Implements C++20 std::assume_aligned from p1007:
http://wg21.link/p1007

Only Clang is initially supported.

Diff Detail

Event Timeline

gonzalobg requested review of this revision.Aug 30 2021, 1:06 AM
gonzalobg created this revision.
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 30 2021, 1:06 AM
gonzalobg updated this revision to Diff 369392.Aug 30 2021, 1:11 AM

Fix missing N in __builtin_assume_aligned

Dup of D54966 - that one seems stuck, do the concerns raised there no longer apply?

gonzalobg updated this revision to Diff 369397.Aug 30 2021, 2:12 AM

Use attribute assume_aligned instead of __builtin_assume_aligned

gonzalobg updated this revision to Diff 369405.Aug 30 2021, 2:28 AM

Remove GCC support, add fail test.

gonzalobg updated this revision to Diff 369407.Aug 30 2021, 3:10 AM

Add missing maybe_unused attributes.

gonzalobg edited the summary of this revision. (Show Details)Aug 30 2021, 3:21 AM
gonzalobg removed a subscriber: lebedev.ri.
gonzalobg updated this revision to Diff 369445.Aug 30 2021, 7:56 AM

nodiscard causes a warning, not an error

gonzalobg updated this revision to Diff 369458.Aug 30 2021, 8:53 AM

Disable -Wgnu-compat for the new tests since this functionality is specific to clang.

ldionne requested changes to this revision.Aug 30 2021, 10:21 AM

Thanks for the patch!

Can you follow the check-list at https://libcxx.llvm.org/Contributing.html (re-generating auto-gen-files, updating status pages, updating synopses, etc.)?

libcxx/include/memory
959

This can be just constexpr since it's C++20-and-above.

960

This should be _LIBCPP_HIDE_FROM_ABI.

961

Please move this to a new header __memory/assumed_aligned.h.

libcxx/test/std/utilities/memory/ptr.align/assume_aligned.fail.cpp
1

Can you please rename this to assume_alligned.verify.cpp?

libcxx/test/std/utilities/memory/ptr.align/assume_aligned.pass.cpp
18–22

We generally follow this pattern:

constexpr bool test() {
  // ... tests go here

  return true;
}

int main(int, char**) {
  test();
  static_assert(test());

  return 0;
}

As to the tests themselves, can you look at https://reviews.llvm.org/D54966 and grab what's relevant? I'm seeing a lot more tests in https://reviews.llvm.org/D54966. For example, we should at least test that assume_aligned returns the same pointer that it is being passed. We should also test with a couple different types, etc.

This revision now requires changes to proceed.Aug 30 2021, 10:21 AM

Also, I just re-read the thread in https://reviews.llvm.org/D54966 (in particular Chandler's comment in https://reviews.llvm.org/D54966#1340163) and my understanding was that we really want to use __builtin_assume_aligned because that will propagate the correct alignment assumption in the IR.

I think the constexpr issue can be solved as:

template <size_t N, typename T>
[[nodiscard]]
_LIBCPP_HIDE_FROM_ABI
constexpr T* assume_aligned(T* __ptr) {
  if (is_constant_evaluated()) {
    return __ptr;
  } else {
    return __builtin_assume_aligned(__ptr, N);
  }
}

As a side benefit, this should also work on GCC (and in fact this implementation is basically what libstdc++ does).

What do you think?

libcxx/include/memory
957

Those parameters should be uglified (<size_t _Np, class _Tp>). Also, we use class instead of typename -- I don't like that, but it's consistent with the rest of the code base.

libcxx/test/std/utilities/memory/ptr.align/assume_aligned.fail.cpp
1

assume_aligned.verify.cpp

Quuxplusone added inline comments.
libcxx/include/memory
968

@ldionne wrote:

template<size_t _Np, class _Tp>
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI
constexpr _Tp *assume_aligned(_Tp *__ptr) {
    if (is_constant_evaluated()) {
        return __ptr;
    } else {
        return static_cast<_Tp *>(__builtin_assume_aligned(__ptr, _Np));
    }
}

AIUI, the non-constexpr-ness problem is due entirely to that static_cast from void* back to _Tp*. ("Constexpr casts are like the Bay Bridge: free in one direction but not the other.") Did anyone think about this during the design of __builtin_assume_aligned? Could we just convince the compiler folks to fix it? The obvious solution is that __builtin_assume_aligned, just like std::assume_aligned, should return the same type that is passed in. Compiler builtins deal with way harder problems than "return the same type" on a daily basis; this should be a piece of cake, I'd think. And then we could use the builtin (and get the optimization assumption) even in constant-folded contexts.

template<size_t _Np, class _Tp>
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI
constexpr _Tp *assume_aligned(_Tp *__ptr) {
#ifdef HAS_SANE_BUILTIN_SEMANTICS
    return __builtin_assume_aligned(__ptr, _Np);
#else
    if (is_constant_evaluated()) {
        return __ptr;
    } else {
        return static_cast<_Tp *>(__builtin_assume_aligned(__ptr, _Np));
    }
#endif
}
ldionne commandeered this revision.Feb 3 2022, 12:26 PM
ldionne edited reviewers, added: gonzalobg; removed: ldionne.

Commandeering to abandon since this is superseded by D118938.

ldionne abandoned this revision.Feb 3 2022, 12:26 PM

(Thanks for the patch, I did keep some content in D118938 and I credit you for it)