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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D79279#2169522 <https://reviews.llvm.org/D79279#2169522>, @jfb wrote:

> In D79279#2168649 <https://reviews.llvm.org/D79279#2168649>, @rjmccall wrote:
>
> > In D79279#2168533 <https://reviews.llvm.org/D79279#2168533>, @jfb wrote:
> >
> > > In D79279#2168479 <https://reviews.llvm.org/D79279#2168479>, @rjmccall wrote:
> > >
> > > > Is there a need for an atomic memcpy at all?  Why is it useful to allow this operation to take on "atomic" semantics — which aren't actually atomic because the loads and stores to elements are torn — with hardcoded memory ordering and somewhat arbitrary rules about what the atomic size is?
> > >
> > >
> > > Hans lays out a rationale for usefulness in his paper, but what I've implemented is more useful: it's *unordered* so you can fence as you desire around it, yet it guarantees a minimum memory access size based on the pointer parameters. For example, copying an atomic `int` will be 4 byte operations which are single-copy-atomic, but the accesses from one `int` to the next aren't performed in any guaranteed order (or observable in any guaranteed order either). I talked about this with him a while ago but IIRC he wasn't sure about implementation among other things, so when you asked me to widen my original `volatile`-only `memcpy` to also do other qualifiers, I realized that it was a neat way to do atomic as well as other qualifiers. I've talked to a few SG1 folks about this, and I believe (for other reasons too) it's where the design will end up for Hans' paper.
> >
> >
> > I can see the usefulness of this operation, but it seems like a odd semantic mismatch for what is basically just a memcpy where one of the pointers happens to have `_Atomic` type, like you're shoe-horning it into this builtin just to avoid declaring a different one.
>
>
> I'm following the discussion we had here regarding overloading <http://lists.llvm.org/pipermail/cfe-dev/2020-May/065388.html>:
>
> >> There are other qualifiers that can meaningfully contribute to the operation here besides volatile, such as restrict and (more importantly) address spaces. And again, for the copy operations these might differ between the two pointer types.
> >> 
> >> In both cases, I’d say that the logical design is to allow the pointers to be to arbitrarily-qualified types. We can then propagate that information from the builtin into the LLVM intrinsic call as best as we’re allowed. So I think you should make builtins called something like __builtin_overloaded_memcpy (name to be decided) and just have their semantics be type-directed.
> > 
> > Ah yes, I’d like to hear what others think of this. I hadn’t thought about it before you brought it up, and it sounds like a good idea.
>
> As you noted earlier, for `memcpy` you probably want to express differences in destination and source qualification, even if today IR can't express e.g. `volatile` source and non-`volatile` destination. You were talking about `volatile`, but this applies to the entire combination of dst+src qualified with zero-to-five `volatile`, `_Atomic`, `__unaligned`, `restrict`, and address space. Pulling the entire combination space out into different functions would create way too many functions. Right now the implementation has a few limitations: it treats both dst and src as `volatile` if either are, it can't do `_Atomic` with `volatile` so we diagnose, and it ignores `restrict`.  Otherwise it supports all combinations.


My point is that this has nothing to do with the ordinary semantics of `_Atomic`.  You're basically just looking at the word "atomic" and saying that, hey, a minimum access size is sortof related to atomicity.

If you want this to be able to control the minimum access size, you should allow that to be passed in as an optional argument instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279