[PATCH] D79279: Add overloaded versions of builtin mem* functions

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D79279#2170157 <https://reviews.llvm.org/D79279#2170157>, @rjmccall wrote:

> I think the argument is treated as if it were 1 if not given.  That's all that ordinary memcpy formally guarantees, which seems to work fine (semantically, if not performance-wise) for pretty much everything today.


I'm not sure that's true: consider a `memcpy` implementation which copies some bytes twice (at different access size, there's an overlap because somehow it's more efficient). That would probably violate the programmer's expectations, and I don't think `volatile` nor atomic `memcpy` allow this (but regular `memcpy` does).

> I don't think you need any restrictions on element size.  It's probably sensible to require the pointers to be dynamically aligned to a multiple of the access width, but I don't think you can enforce that statically.

Agreed, if we're given a short and told to copy 4 bytes at a time then UBSan could find the constraint violation on alignment, but generally the only way we can diagnose is if the parameter is `__unaligned` (because there you're explicitly telling me it's not aligned, and the constraint is that it has to be).

> And of course the length needs to be a multiple of the access size.

Yeah.

> Do you think it'd be useful to have different guarantees for different operands?  I guess it could come up, but it'd be a whole lot of extra complexity that I can't imagine we'd ever support.

You mean, if `element_size` is passed then you get different guarantees? I think that's what makes sense: if you're asking for atomic `memcpy` then you get guarantees. If you're asking for `volatile` `mempcy` then you get others. That's why overloading (and multiple parameters) can be confusing, but at the same time I think it's better than having the combinatorial number of named functions instead.

> If one of the arguments is `volatile`, arguably the minimum access width (if given) needs to be exact.  If we don't support that right now, it's okay to make it an error, which is basically you've already done with the `_Atomic volatile` diagnostic.

Agreed. `volatile` with size makes a lot of sense, and the IR version of it, once created, ought to not be able to widen accesses. `volatile` without a size specified makes sense too, because you just want a single read and a single write, don't care about tearing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79279/new/

https://reviews.llvm.org/D79279