[PATCH] D79279: Allow volatile parameters to __builtin_mem{cpy,move,set}

Sun May 3 11:09:32 PDT 2020

rjmccall added a comment.

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

> In D79279#2016573 <https://reviews.llvm.org/D79279#2016573>, @rjmccall wrote:
>
> > In D79279#2016570 <https://reviews.llvm.org/D79279#2016570>, @rjmccall wrote:
> >
> > > I do think this is a somewhat debatable change in the behavior of these builtins, though.
> >
> >
> > Let me put more weight on this.  You need to propose this on cfe-dev.
>
>
> Happy to do so. Is this more about the change in the builtin, or about spelling it `__builtin_volatile_memcpy` and such? I've thought about this, and when the builtin has two potentially volatile arguments I've concluded that the IR builtin really wasn't sufficient in semantics, but in practice it is sufficient today. So putting `volatile` in a function name (versus overloading) seems to not really be what makes sense here. I'd therefore rather overload, and as you say we could support more than just `volatile` in doing so. Is that the main thing you'd suggest going for in an RFC (`volatile` as well as address space overloads and whatever else)? Again, I'm happy to do that, but I want to make sure I reflect your feedback correctly.

`__builtin_memcpy` is a library builtin, which means its primary use pattern is #define tricks in the C standard library headers that redirect calls to the `memcpy` library function.  So doing what you're suggesting to `__builtin_memcpy` is also changing the semantics of `memcpy`, which is not something we should do lightly.  If we were talking about changing a non-library builtin function, or introducing a new builtin, the considerations would be very different.  If that's what you want to do now, I think that's much easier to justify, although I still think it's worth starting a thread about it to give people an opportunity to weigh in.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279