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

[Johannes Doerfert via Phabricator via llvm-commits]

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

Can you rebase and remove the artifacts in the diff? I also provided final comments. I think once they are fixed it is fine. Feel free to commit a fixed version.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2664
+                               AI->getNextNode());
+        }
+
----------------
Leftover comment and braces around single stmt.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2745
+        if (!CB->isArgOperand(U))
+          continue;
+        unsigned ArgNo = U - CB->arg_begin();
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2765
+      return false;
+    }
+    return true;
----------------
Allow lifetime.start/end as users here as well. That should unbreak one of the test cases.unbreak


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2780
+    } else
+      return true;
+    
----------------
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.


================
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
+
----------------
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.


================
Comment at: llvm/test/Transforms/FunctionAttrs/heap_to_stack.ll:202
+  ret i32 %3
+}
+
----------------
This should be fine. Also a lifetime.end user is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65408