This is an archive of the discontinued LLVM Phabricator instance.

[libc][mem*] Address facility + test enum support
ClosedPublic

Authored by gchatelet on May 19 2022, 4:13 AM.

Details

Summary

This patch is a subpart of D125768 intented to make the review easier.

The Address struct represents a pointer but also adds compile time knowledge
like alignment or temporal/non-temporal that helps with downstream instruction
selection.

Diff Detail

Event Timeline

gchatelet created this revision.May 19 2022, 4:13 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 19 2022, 4:13 AM
gchatelet requested review of this revision.May 19 2022, 4:13 AM
courbet added inline comments.May 19 2022, 4:55 AM
libc/src/string/memory_utils/address.h
25

DeferredStaticAssert for consistency ? Note that as duscussed offline, I would much rather prefer consistency with c++, i.e. renaming everything to cpp::conditional_t instead of cpp:ConditionalType.

32

"specifying" ? (here and below).

34

I would call this Temporality { TEMPORAL, NONTEMPORAL}. Else AddrT::TEMPORAL reads as "a temporal address" rather than "the temporality of the Address type AddrT".

48

Not a big fan of this, because this is spoofable: struct SomeOtherType { static constexpr bool IS_ADDRESS_TYPE = true; };. Is this on purpose ? Else I think template <typename T> IsAddressType { constexpr bool value = false; }; + specialization would be better.

54

PointeeType ? I could see ValueType being ubyte* or ubyte. Using Pointee makes it non-ambiguous.

55

VoidType ?

67

Maybe mention that this is not UB because value_type is a character type ? This triggers alarms in my brain :)

75

The doc should mention whether the offsets are in bytes or multiple of alignment. As of now I have to read the code to figure out.

77

I don't understand why an address offset by AddrT::ALIGNMENT+1 should be AddrT::ALIGNMENT-aligned. is this missing a static_assert(Offset <= AddrT::ALIGNMENT) ?

90

(in bytes)

95

ditto

gchatelet updated this revision to Diff 433036.May 31 2022, 1:14 AM
gchatelet marked 11 inline comments as done.
  • Address comments
courbet added inline comments.May 31 2022, 1:43 AM
libc/src/string/memory_utils/address.h
75

This is suboptimal for ALIGNMENT=8 and ByteOffset=6, which has ByteOffsetModuloAlignment=6, and therefore returns 1 instead of 2.

libc/test/src/string/memory_utils/address_test.cpp
48

ASSERT_EQ(offsetAddr<6>(SrcAddr<8>(nullptr)).ALIGNMENT, 2UL); ?

gchatelet updated this revision to Diff 433041.May 31 2022, 2:34 AM
  • Use a more principled approach to common alignment
gchatelet marked 2 inline comments as done.May 31 2022, 2:35 AM
courbet accepted this revision.May 31 2022, 5:36 AM
This revision is now accepted and ready to land.May 31 2022, 5:36 AM
gchatelet updated this revision to Diff 433315.Jun 1 2022, 1:29 AM
  • Simplify gcd in address
This revision was landed with ongoing or failed builds.Jun 1 2022, 2:10 AM
This revision was automatically updated to reflect the committed changes.