[PATCH] D74651: Add IR constructs for inalloca replacement preallocated call setup

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Thanks for reviewing Eli! I had some wording suggestions and syntax nits worth reuploading for.



================
Comment at: llvm/docs/LangRef.rst:2203
+      %t = call token @llvm.call.preallocated.setup(i32 1)
+      %a = call i8* @llvm.call.preallocated.arg(i32 0)
+      %b = bitcast i8* %a to %foo*
----------------
This should take the token as the first argument. I guess now it should also use the new call site attribute as well.


================
Comment at: llvm/docs/LangRef.rst:11951
+      %t2 = @llvm.call.preallocated.setup(i32 0)
+      call void foo() ["preallocated(token %t2)]
+      call void foo() ["preallocated(token %t1)]
----------------
You should close the bundle name string in the example, `"preallocated"(token...`


================
Comment at: llvm/docs/LangRef.rst:11973
+
+      declare i8* @llvm.call.preallocated.arg(token %setup_token, i32 %arg_index)
+
----------------
We discussed the need to add the argument size here


================
Comment at: llvm/docs/LangRef.rst:1971-1977
+    and cannot be used on any other call. The attribute type must be the
+    actual type of the call argument. This is used in the case that an
+    ``llvm.call.preallocated.setup`` does not have a corresponding call, which
+    would result in the stack being adjusted without the call readjusting. The
+    ``llvm.call.preallocated.setup`` is eliminated and all corresponding
+    ``llvm.call.preallocated.arg``s are changed to ``alloca``s of the proper
+    type.
----------------
I think this wording could be improved. I made a draft, didn't like it, and threw it away. :(

I think "actual argument" is vague. I tried to say something like, "The type used on this attribute has to match the type used by the attribute of the corresponding argument at the final call site."

I think it might be better to take the rest of this documentation and move it to the description of the intrinsic, and then refer to it here, after documenting the requirement on the attribute type.

I'd replace the fragment "which would result in the stack being adjusted..." with something about the fact that, without a call site, we can't do any stack adjustments at all, since we don't know the argument types, and therefore don't know how much to adjust the stack by.

>From the standpoint of an optimizer, `llvm.call.preallocated.arg` is a special alloca: it allocates stack memory with some type. This attribute carries that type. If there is no corresponding call site, then it is legal and desirable to transform these intrinsics to plain static allocas using the type on this attribute.


================
Comment at: llvm/docs/LangRef.rst:2200
+non-trivially copyable objects by value in a way that is compatible with MSVC
+on Win32.  There can be at most one ``"preallocated"`` operand bundle
+attached to a call site and it must have exactly one bundle operand, which is
----------------
I guess I wrote the words on "on Win32", but perhaps we should make it more vague and say "... compatible with MSVC on some targets." I think this call setup stuff is also necessary on arm32/thumb.


================
Comment at: llvm/docs/LangRef.rst:2204
+operand bundle should not adjust the stack before entering the function, as
+that will have been done by one of the `@llvm.call.preallocated.*` intrinsics.
+
----------------
I think .rst files need double backticks, or it's a link, not monospace text.


================
Comment at: llvm/docs/LangRef.rst:11959
+
+      %t1 = @llvm.call.preallocated.setup(i32 0)
+      %t2 = @llvm.call.preallocated.setup(i32 0)
----------------
You have to add `call token` between `= @` here to make it legal-ish IR


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74651