Skip to content

<xmemory>: Investigate constexpr approach between _Default_allocator_traits::allocate and _Allocate #1532

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

Open
mnatsuhara opened this issue Dec 14, 2020 · 3 comments
Labels
enhancement Something can be improved

Comments

@mnatsuhara
Copy link
Contributor

mnatsuhara commented Dec 14, 2020

In GH-1369, we will be implementing is_constant_evaluated() pathways in both _Default_allocator_traits::allocate and the global _Allocate function that is called from that function if not evaluated in a constexpr context.

_Default_allocator_traits::allocate

STL/stl/inc/xmemory

Lines 661 to 673 in ac4fde7

_NODISCARD static _CONSTEXPR20_DYNALLOC __declspec(allocator) pointer
allocate(_Alloc& _Al, _CRT_GUARDOVERFLOW const size_type _Count) {
#ifdef __cpp_lib_constexpr_dynamic_alloc // TRANSITION, GH-1532
if (_STD is_constant_evaluated()) {
return _Al.allocate(_Count);
} else
#endif // __cpp_lib_constexpr_dynamic_alloc
{
(void) _Al;
return static_cast<pointer>(
_Allocate<_New_alignof<value_type>>(_Get_size_of_n<sizeof(value_type)>(_Count)));
}
}

_Allocate:

STL/stl/inc/xmemory

Lines 173 to 198 in ac4fde7

// FUNCTION TEMPLATES _Allocate and _Deallocate
#ifdef __cpp_aligned_new
template <size_t _Align, class _Traits = _Default_allocate_traits,
enable_if_t<(_Align > __STDCPP_DEFAULT_NEW_ALIGNMENT__), int> = 0>
__declspec(allocator) _CONSTEXPR20_DYNALLOC void* _Allocate(const size_t _Bytes) {
// allocate _Bytes when __cpp_aligned_new && _Align > __STDCPP_DEFAULT_NEW_ALIGNMENT__
if (_Bytes == 0) {
return nullptr;
}
#ifdef __cpp_lib_constexpr_dynamic_alloc // TRANSITION, GH-1532
if (_STD is_constant_evaluated()) {
return _Traits::_Allocate(_Bytes);
} else
#endif // __cpp_lib_constexpr_dynamic_alloc
{
size_t _Passed_align = _Align;
#if defined(_M_IX86) || defined(_M_X64)
if (_Bytes >= _Big_allocation_threshold) {
// boost the alignment of big allocations to help autovectorization
_Passed_align = (_STD max)(_Align, _Big_allocation_alignment);
}
#endif // defined(_M_IX86) || defined(_M_X64)
return _Traits::_Allocate_aligned(_Bytes, _Passed_align);
}
}

Currently, MSVC cannot handle calling the _Allocate function from a constexpr context (_Allocate calls ::operator new which MSVC does not allow in a constexpr function, though Clang does if it is reached transitively from an evaluation of std::allocator::allocate), which is why there is the is_constant_evaluated() block in _Default_allocator_traits::allocate that instead dispatches to std::allocator::allocate (which is permitted by MSVC). Note that _Normal_allocator_traits is not affected by this issue because it does not attempt the optimization at directly calling _Allocate and instead always goes through _Alloc::allocate.

It seems like we should either (1) have _Allocate be completely constexpr-safe (have valid constexpr pathways for all supported compilers), or (2) have _Allocate be a completely non-constexpr function that is just never called in constexpr pathways (and in this case, I believe we could also remove the constexpr pathways in _Default_allocate_traits used in _Allocate, _Deallocate). This depends on whether MSVC will permit the call to _Allocate in the future as the constexpr dynamic allocation implementation matures (in this case, we could adopt approach (1)). Otherwise, we can look into option (2). Regardless, we should pick one of these options to avoid this code duplication and the logic being split up in this way.

Internal Microsoft link only: You can hear more discussion about this issue here: https://msit.microsoftstream.com/video/4740a1ff-0400-b9eb-093e-f1eb3b56cd9a?st=2877

@mnatsuhara mnatsuhara added the enhancement Something can be improved label Dec 14, 2020
@cdacamar
Copy link
Contributor

From a purely language semantics point of view: MSVC is doing the right thing. The standard states the following are allowed in constant expressions [expr.const]/5.17:

  • a new-expression ([expr.new]), unless the selected allocation function is a replaceable global allocation function ([new.delete.single], [new.delete.array]) and the allocated storage is deallocated within the evaluation of E;
  • a delete-expression ([expr.delete]), unless it deallocates a region of storage allocated within the evaluation of E;
  • a call to an instance of std​::​allocator​::​allocate ([allocator.members]), unless the allocated storage is deallocated within the evaluation of E;
  • a call to an instance of std​::​allocator​::​deallocate ([allocator.members]), unless it deallocates a region of storage allocated within the evaluation of E;

The expression ::operator new is not a new expression, it is a function call to global operator new() which is not a constexpr function.

@mnatsuhara
Copy link
Contributor Author

Yes, I think it was perhaps incorrect to say that _Allocate is "broken" for MSVC, since the Standard doesn't require _Allocate's definition to be permitted, Clang's implementation choices just happen to make it so. I think if MSVC maintains its current approach, continuing to consider _Allocate as off-limits in a constexpr context, then we should just lean in the direction of option (2) here.

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Dec 15, 2020

The standard states the following are allowed in constant expressions [expr.const]/5.17:

@cdacamar I think you mean the exact opposite - those evaluations are forbidden in a constant expression. For example, "a call to an instance [I am not fond of the use of "instance" here to refer to the result of instantiating a function template] of std​::​allocator​::​deallocate" is forbidden "unless it deallocates a region of storage allocated within the evaluation of [the constant expression] E".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

No branches or pull requests

3 participants