This is an archive of the discontinued LLVM Phabricator instance.

[libc][mem*] Introduce Algorithms for new mem framework
ClosedPublic

Authored by gchatelet on Jun 22 2022, 4:37 AM.

Details

Summary

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

This patch introduces the same algorithms as in libc/src/string/memory_utils/elements.h but using the new API.

Diff Detail

Event Timeline

gchatelet created this revision.Jun 22 2022, 4:37 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 22 2022, 4:37 AM
gchatelet requested review of this revision.Jun 22 2022, 4:37 AM

First comments: statically sized operations.

libc/src/string/memory_utils/algorithm.h
18

// See each class for documentation.

31

I don't see Skip being tested, or even used. Is it useful ?

97

Please format the patch.

108

I would say "the first and the last SizedOp::Size bytes of the buffer". That way it's clear that the SizedOp has to be a statically sized operation.

119

maybe:

static_assert(SizedOp::SIZE >0);

to fail explicitly if SizedOp is not a statically sized operation.

128

//Loads and stores cannot be interleaved because they might alias.

130

Do you want to comment on the order of the stores ? Both are possible provided that they come after both loads. Is there a reason to chose one over the other ? Same for loads actually.

143

Can you comment on using short-circuiting/branching vs |.

149

Does this one even make sense if runtime_size > 2*SizedOp::SIZE ? Maybe add an assert ?

courbet added inline comments.Jun 23 2022, 7:00 AM
libc/src/string/memory_utils/algorithm.h
143

please use a type.

151

ditto

170

This is a different concept compared to e.g. Head::SIZE: would it make sense to call that BLOCK_SIZE ?

251

ditto (here and everywhere for int32_t and uint64_t)

gchatelet updated this revision to Diff 440125.Jun 27 2022, 1:37 AM
gchatelet marked 13 inline comments as done.
  • Address comments
gchatelet added inline comments.
libc/src/string/memory_utils/algorithm.h
31
119

By definition SizedOp is a statically sized operation

130

I still don't fully understand the ordering consequences so I made it clear by adding a comment. Thx for the suggestion.

149

It does not make sense but I think we cannot use assert inside memcpy and other low level functions as assert could use them under the hood.
+ @sivachandra for confirmation.

sivachandra added inline comments.Jun 27 2022, 1:46 AM
libc/src/string/memory_utils/algorithm.h
149

Yes, for now lets avoid asserts. I will get back to you with a detailed answer in the coming weeks.

courbet added inline comments.Jun 27 2022, 2:08 AM
libc/src/string/memory_utils/algorithm.h
119

That's what I want to check :)

I think that you essentially have two kinds of operations:

  • What I was calling "statically sized operations": _1, _2, ..., ``.

Maybe a better name would be "Composite operations" for the later and "Elementary operations" for the former.

My point is that I think some operations only take elementary operations, for example, this does not really make sense:

HeadTail<Loop<_16>>

We might even want static constexpr bool IS_ELEMENTARY = true, or something like this.

WDYT ?

130

I think this should be BLOCK_SIZE too.

149

OK, can you at least add a comment ?

288

"Aligns using a statically-sized SizedOp operation, then calls..."

291

16 is not a SizedOp: this is not consistent with the code below.

387

I think this should be named ALIGN_OP_SIZE or something like this.

courbet added inline comments.Jun 27 2022, 2:16 AM
libc/src/string/memory_utils/algorithm.h
119

Actually an elementary SizedOp is an instance of SizeOp<BackendT, Size>. So we could have a IsElementaryOp<SizedOpT> that checks that.

121

Maybe use a different name, e.g. SizedOpT ? SizedOp is already the name of a template, and name collisions are always hard to debug (here and everywhere else).

gchatelet updated this revision to Diff 440225.Jun 27 2022, 7:46 AM
gchatelet marked 8 inline comments as done.
  • Address comments
libc/src/string/memory_utils/algorithm.h
119

I went with the static constexpr bool member variable as it's simpler. It's hackable from the outside but this is not a concern here, actually it's fine to use a different type as long as it duck types.

gchatelet marked an inline comment as done.Jun 27 2022, 7:46 AM
gchatelet updated this revision to Diff 440228.Jun 27 2022, 7:51 AM
  • Address comments
courbet accepted this revision.Jun 28 2022, 2:19 AM
This revision is now accepted and ready to land.Jun 28 2022, 2:19 AM
This revision was automatically updated to reflect the committed changes.