[PATCH] D77689: [X86] Codegen for preallocated

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

I have some surface level code style comments, please address those and land this if you agree.

The test case raises an interesting issue: how to handle musttail thunks. However, that will require verifier and maybe langref changes, so let's address that separately.



================
Comment at: llvm/lib/Target/X86/X86MachineFunctionInfo.h:199
+
+  size_t PreallocatedIdForCallSite(const Value *CS) {
+    auto Insert = PreallocatedIds.insert({CS, PreallocatedIds.size()});
----------------
LLVM is inconsistent about the casing of names, but this class seems to consisitently use leadingLowerCamelCase, so let's do the same here and below, unless you see a reason not to.


================
Comment at: llvm/lib/Target/X86/X86MachineFunctionInfo.h:217
+
+  void SetPreallocatedArgOffsets(size_t Id, SmallVector<size_t, 4> AO) {
+    PreallocatedArgOffsets[Id] = AO;
----------------
Generally, we try not to pass `SmallVector` by value. It is only small in the sense that we believe they contain few elements. Embedding storage in the vector makes it large, and expensive to copy. Generally to pass in a list of things, we tend to prefer `ArrayRef<size_t>`, so the caller can be flexible about the collection they are using, as long as its a flat array.


================
Comment at: llvm/lib/Target/X86/X86MachineFunctionInfo.h:221
+
+  const SmallVector<size_t, 4> &GetPreallocatedArgOffsets(const size_t Id) {
+    assert(!PreallocatedArgOffsets[Id].empty() && "arg offsets not set");
----------------
Similarly, to give the caller a readonly view of the offsets, I would recommend ArrayRef.


================
Comment at: llvm/test/CodeGen/X86/musttail-indirect.ll:58
 
+; FIXME: This generates a lot of code even at -O2, any better way to do this? Same with all the preallocated versions of functions below.
+; CHECK-LABEL: g_thunk_2:
----------------
This is interesting, we forgot about this when writing the LangRef.

If you look at the previous inalloca test, it forwards its inalloca parameter directly to the musttail callee. It doesn't allocate new memory. That's the real use case for this `musttail` thing, and we probably need to bend the verifier rules to allow direct forwarding of preallocated arguments to a musttail call site.

I think for now, committing this as is with a FIXME and coming back to it later would be fine. I assume that the verifier rejects it if you remove the setup and arg calls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77689