[PATCH] D47345: [InstructionCombining] Replace small allocations with local/global variable

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

The biggest problem I see with this is the potential to cause a stack overflow; you've specified a limit of 100 bytes per allocation, but there's no limit to the number of allocations. Not sure what the right solution is; LLVM doesn't really have good heuristics here.



================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2267
+  // Threshold whether change malloc to alloca or not
+  static unsigned FoldTreshold = 100;
+
----------------
Please make this an option, so it can be tweaked for debugging.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2299
+
+  if (LI->getLoopFor(Malloc->getParent())) {
+    // Do not optimize if malloc is in loop
----------------
This isn't sufficient to guarantee malloc will be called once; getLoopFor won't find irreducible loops.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2314
+  Builder.SetInsertPoint(Malloc->getParent(), ++Malloc->getIterator());
+  Value *LocAlloc = Builder.CreateAlloca(Builder.getInt8Ty(), Size);
+  Malloc->replaceAllUsesWith(LocAlloc);
----------------
You probably don't want to insert an alloca in the middle of a function; allocas outside the entry block are treated as dynamic allocations, so they're slower, and won't work the way you want with code that uses llvm.stackrestore.


https://reviews.llvm.org/D47345