[PATCH] D57635: Add a new intrinsic and attribute to implement `__builtin_va_arg_pack{, _len}` in Clang

[Erik Pilkington via Phabricator via llvm-commits]

erik.pilkington marked 2 inline comments as done.
erik.pilkington added a comment.

In D57635#1382135 <https://reviews.llvm.org/D57635#1382135>, @efriedma wrote:

> Maybe a silly question, but is this actually worth implementing, at this point?  The new IR additions have unusual semantics that transforms are continually going to trip over; I'm not convinced you found all the places that currently need checks.  And there isn't any existing code we need to be compatible with, I think.  If we were designing a frontend extension from scratch, we would probably take a different approach.


What kind of approach are you thinking? For us, this isn't about being compatible with GCC as much as it is about getting the feature. Doing something different here would be fine as long as its "better enough" to justify deviating from GCC compatibility. Even though no existing code depends on it, compatibility with GCC would still be nice to have, so existing libraries that conditionally use the `__builtin_va_arg_pack` will just work with clang, whereas if we went with a different solution they would have to keep it disabled or write two copies of all their wrapper functions.



================
Comment at: llvm/docs/LangRef.rst:1408
+    arguments of the caller will be forwarded into the call instruction. This
+    attribute is used to implement `__builtin_va_arg_pack`.
 ``jumptable``
----------------
efriedma wrote:
> I'm not really a fan of using an attribute like this.  Currently, there are basically two ways to refer to a varargs list: va_start, and a musttail call. This is adding a third, in a way that's very easy to miss.
> 
> Not sure what it should look like, though.  Maybe instead of calling the function directly, we should call an intrinsic?  I guess any approach has odd semantic implications.
I tried this out first with an intrinsic, but it seemed like pretending that the va args were a value produced by a call that are passed as an argument in a different instruction was a more fragile and odd way of modelling this. Either way though, there will still be 3 ways of referencing va args, and I'm not sure that using an intrinsic instead of an attribute would make this corner case any more visible.


================
Comment at: llvm/docs/LangRef.rst:10475
+difference of the total number of arguments at the call site and the number of
+declared parameters.
+
----------------
efriedma wrote:
> You're computing the difference here using the number of IR arguments?  That's not going to return the correct result in general; you need the frontend to compute this.
> 
> -------
> 
> I wonder if we should add an attribute to denote "weird" intrinsics that can't be treated as equivalent to a function call in general.  (For example, I think you have to forbid function merging on any function that calls this intrinsic.)
> You're computing the difference here using the number of IR arguments? That's not going to return the correct result in general; you need the frontend to compute this.

Oh right, good point. I'll update this on monday.

> I wonder if we should add an attribute to denote "weird" intrinsics that can't be treated as equivalent to a function call in general. (For example, I think you have to forbid function merging on any function that calls this intrinsic.)

What other cases do you think are "weird" enough?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57635