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

[Stefan Stipanovic via Phabricator via llvm-commits]

sstefan1 marked 3 inline comments as done.
sstefan1 added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2745
+        if (!CB->isArgOperand(U))
+          continue;
+        unsigned ArgNo = U - CB->arg_begin();
----------------
jdoerfert wrote:
> this has to lead to invalidation. use in a operand bundle could free even if the function is nofree. Maybe move the check to the beginning of the `CallBase` conditional.
It makes sense to me to move the check to the beginning. 

> use in a operand bundle could free even if the function is nofree
I'm not I understand this, though.




================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2780
+    } else
+      return true;
+    
----------------
jdoerfert wrote:
> You have to put them into the "Bad" instruction set here. Also, move the "is bad" check to the beginning.
> There is a test missing for a malloc and a calloc that have to large sizes. (calloc only the multiplication should be too large).
> We need to check that the multiplication does not overflow. APInt has a method to do overflow checked multiplication.
> Also, move the "is bad" check to the beginning.
If you mean in initialize, then I'd say this is a nicer approach.




================
Comment at: llvm/test/Transforms/FunctionAttrs/heap_to_stack.ll:1
+; RUN: opt -passes=attributor --attributor-disable=false -enable-heap-to-stack-conversion=true -S < %s | FileCheck %s
+
----------------
jdoerfert wrote:
> When we run this with the old PM, what will happen? No h2s? We should do it and make sure it doesn't crash etc.
Yes, no h2s and doesn't crash.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65408