[PATCH] D77689: [X86] Codegen for preallocated

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

I think the approach looks good, sorry for the delay.

In D77689#2020636 <https://reviews.llvm.org/D77689#2020636>, @aeubanks wrote:

> Is there a way to automatically update the Phab change with the commit message if you're only uploading one commit?


I don't think so, I think the only way to update the commit message is through the web interface.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5810
+           "expected call_preallocated_setup Value");
+    const CallBase *Call = nullptr;
+    for (auto U : PreallocatedCall->users()) {
----------------
The common code here seems to be this search of the use list to find the call that consumes the setup. I'd suggest factoring that out into a helper. Once that is done, it seems like this code might be simpler if you separate the setup and arg cases.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:3887
+    if (!ArgLocs.back().isMemLoc()) {
+      report_fatal_error("cannot use preallocated attribute on a register "
+                         "parameter");
----------------
This seems like it could be an assert, but I could go either way. If the condition occurs, it's an LLVM bug: it means the x86 calling conv code missed the case. We generally use report_fatal_error when user input could trigger the condition, and we don't want UB to occur.


================
Comment at: llvm/lib/Target/X86/X86MachineFunctionInfo.h:109
+
+  size_t PreallocatedNextId = 0;
+  ValueMap<const Value *, size_t> PreallocatedIds;
----------------
This could be tracked as `PreallocatedStackSizes.size()`, perhaps.


================
Comment at: llvm/lib/Target/X86/X86MachineFunctionInfo.h:111
+  ValueMap<const Value *, size_t> PreallocatedIds;
+  DenseMap<size_t, size_t> PreallocatedStackSizes;
+  DenseMap<size_t, SmallVector<size_t, 4>> PreallocatedArgOffsets;
----------------
I tend to micro-optimize data structures, but they keys to this are densely packed integers, so this could be a vector instead of a hash map.


================
Comment at: llvm/lib/Target/X86/X86MachineFunctionInfo.h:112
+  DenseMap<size_t, size_t> PreallocatedStackSizes;
+  DenseMap<size_t, SmallVector<size_t, 4>> PreallocatedArgOffsets;
+
----------------
DenseMap is open-addressed, so this takes up more memory than you might imagine. If you change it to SmallVector, the common case is that there are no call sites using preallocated, so I'd use 0 builtin elements.


================
Comment at: llvm/lib/Target/X86/X86MachineFunctionInfo.h:203
+      size_t NewId = PreallocatedNextId++;
+      PreallocatedIds.insert({CS, NewId});
+      return NewId;
----------------
Can this function use the get-or-insert pattern:
  auto Insert = PreallocatedIds.insert({CS, PreallocatedNextId});
  if (Insert.second)
    ++PreallocatedNextId;
  return Insert.first->second;


================
Comment at: llvm/lib/Target/X86/X86MachineFunctionInfo.h:215
+  size_t GetPreallocatedStackSize(const size_t Id) {
+    assert(PreallocatedStackSizes.find(Id) != PreallocatedStackSizes.end() &&
+           "stack size not set");
----------------
You can shorten this with `.count(Id)`


================
Comment at: llvm/lib/Target/X86/X86MachineFunctionInfo.h:225
+  const SmallVector<size_t, 4> &GetPreallocatedArgOffsets(const size_t Id) {
+    assert(PreallocatedArgOffsets.find(Id) != PreallocatedArgOffsets.end() &&
+           "arg offsets not set");
----------------
You can shorten this with `.count(Id)`


================
Comment at: llvm/lib/Target/X86/X86RegisterInfo.cpp:629
 
 bool X86RegisterInfo::hasBasePointer(const MachineFunction &MF) const {
+  const X86MachineFunctionInfo *X86FI = MF.getInfo<X86MachineFunctionInfo>();
----------------
Ooh, base pointers are expensive. Is this necessary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77689