Page MenuHomePhabricator

[Android] Set NewAlign for 64-bit Android to 8 bytes
AbandonedPublic

Authored by pirama on Aug 13 2018, 5:53 PM.

Details

Summary

Android uses jemalloc allocator, which returns 8-byte-aligned pointers for
allocations smaller than 8 bytes for 64-bit architectures. Set NewAlign
conservatively to 8 bytes.

Diff Detail

Event Timeline

pirama created this revision.Aug 13 2018, 5:53 PM
efriedma accepted this revision.Aug 14 2018, 11:11 AM
efriedma added a subscriber: efriedma.

LGTM

lib/Basic/TargetInfo.cpp
72

Might as well just set NewAlign = 64; here. But not a big deal either way.

This revision is now accepted and ready to land.Aug 14 2018, 11:11 AM
rsmith requested changes to this revision.Aug 15 2018, 1:24 PM

This doesn't seem necessary. NewAlign specifies the alignment beyond which types acquire "new-extended alignment" per the C++ standard, or equivalently the alignment beyond which we need to pass an align_val_t argument to operator new.

If all types of size <= 8 are provided with sufficiently-aligned storage (which 8 byte alignment definitely is), then they are irrelevant for the computation of this value, because new T for such a type never needs to pass an alignment. (A similar argument applies for the array-new case.)

This revision now requires changes to proceed.Aug 15 2018, 1:24 PM

The C++ standard just says "An integer literal of type std::size_t whose value is the alignment guaranteed by a call to operator new(std::size_t) or operator new[](std::size_t)." I would take that in the obvious sense, that any pointer returned by operator new must have alignment __STDCPP_DEFAULT_NEW_ALIGNMENT__.

Granted, I can see how that might not be the intent, given that alignof(T) >= sizeof(T) for all C++ types.

pirama abandoned this revision.Aug 20 2018, 11:04 AM

Thanks for the clarification Richard and Eli. I agree that leaving the status quo will match the intent of the macro. I'll abandon this.