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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

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.



================
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``
----------------
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.


================
Comment at: llvm/docs/LangRef.rst:10475
+difference of the total number of arguments at the call site and the number of
+declared parameters.
+
----------------
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.)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57635