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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

There are some leftover comments but this is almost ready.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:129
+                                       cl::init(1024), cl::Hidden);
+
 /// Logic operators for the change status enum class.
----------------
I'd enable this by default but for a smaller size, let's say 128?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2652
+        BC->insertAfter(AI);
+        AI = BC;
+      }
----------------
`AI = new BitCastInst(AI, MallocCall->getType(), "malloc_bc", AI->getNextNode());`

might do the trick.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2673
+  /// Collection of all malloc calls in a function.
+  SmallSetVector<const Instruction *, 4> MallocCalls;
+
----------------
remove the const here then you can avoid the const casts


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2719
+            A.getAAFor<AANoFree>(*this, IRPosition::callsite_function(*CB));
+        if (NoFreeAA.isAssumedNoFree())
+          continue;
----------------
If no-free is set nocaputre is not checked anymore. We need to check for nofree and nocapture.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2724
+        // argument is not freed.
+        if(CB->isArgOperand(U)){
+          unsigned ArgNo = U - CB->arg_begin();
----------------
If this is not the arg operand we should give up. (we may allow it at the callee position but not in the operand bundle)


================
Comment at: llvm/test/Transforms/FunctionAttrs/heap_to_stack.ll:58
+
+; TEST 4 - negative, no call to free
+
----------------
outdated comment.


================
Comment at: llvm/test/Transforms/FunctionAttrs/heap_to_stack.ll:68
+
+; TEST 5 - not all exit paths have a call to free
+
----------------
maybe elaborate why h2s is still OK.


================
Comment at: llvm/test/Transforms/FunctionAttrs/heap_to_stack.ll:103
+  ; CHECK-NOT: @free(i8* %2)
+  br label %5
+
----------------
You want branch %6 or you would have a double free (not necessarily a problem for h2s but still).


================
Comment at: llvm/test/Transforms/FunctionAttrs/heap_to_stack.ll:121
+  tail call i32 @no_return_call()
+  ; this free is dead. So malloc cannot be transformed.
+  ; CHECK-NOT: @free(i8* %1)
----------------
leftover comment.


================
Comment at: llvm/test/Transforms/FunctionAttrs/heap_to_stack.ll:127
+
+; TEST 8 - Negative: bitcast pointer used in capture function
+
----------------
We need this test again with an `nounwind` `@foo` which should allow h2s.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65408