[PATCH] D65408: [Attributor] Heap-To-Stack Conversion

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2073
+      return;
+    }
+
----------------
Not needed, see my comment further below.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2078
+    for (auto &BB : F)
+      for (auto &I : BB) {
+        if (isMallocLikeFn(&I, TLI))
----------------
Not needed, we can use the InfoCache to look only at calls.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2084
+              LLVM_DEBUG(dbgs() << "H2S: Initial malloc call: " << I << "\n");
+            }
+
----------------
Let's collect all valid h2s candidate malloc calls and then only "materialize" the ones we deem profitable. Knowing a malloc could be converted to the stack is already a nice piece information.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2301
+    // Check all the paths. If we find one path that doesn't free or an exit
+    // that captures the pointer, we are done.
+    while (!WorkList.empty()) {
----------------
If you want to start with a small patch, just check the function for `willreturn`, `nounwind`, and `nofree`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2328
+      BadMallocCalls.push_back(MallocCall);
+  }
+
----------------
I dislike this exploration. We should not need to traverse all paths all the time. (This does not track visited blocks btw. so it is probably an endless loop if there are loops in the function).

We should (later) follow the uses of the malloc only. The logic I have in mind is as follows (IMHO much easier than this (=the old logic)):

If all (transitive) users of a `malloc` are either `nocapture` uses or `free` uses, we can do heap2stack. In fact, we should probably generalize the capture AA to collect capturing uses instead so we can analyze them later, e.g., here.



================
Comment at: llvm/test/Transforms/FunctionAttrs/heap_to_stack.ll:17
+
+declare void @free(i8* nocapture)
+
----------------
I think this is a bug, see below.

I'll create a patch to *not* annotate `free` with `nocapture`.


================
Comment at: llvm/test/Transforms/FunctionAttrs/heap_to_stack.ll:25
+  ; CHECK-NEXT: @nocapture_func_frees_pointer(i8* %1)
+  tail call void @nocapture_func_frees_pointer(i8* %1)
+  tail call void (...) @func_throws()
----------------
I'd argue, "freeing" a pointer is "capturing" by nature so this combination "nocapture" + "free" cannot exist. If it doesn't, "nocapture" is marked and implies "nofree" on the pointer. Thus, we can actually allocate `%1` on the stack here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65408