[PATCH] D81765: Don't hoist very large static allocas to the entry block during inlining

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

What happens if the alloca that is left in the conditional instead of being moved at the caller's entry block is on the hot path, like in a loop?



================
Comment at: llvm/include/llvm/Analysis/InlineCost.h:54
+/// size is above this amount in bytes.
+const unsigned MaxStaticAllocaSizeToMove = 65536;
 } // namespace InlineConstants
----------------
MadCoder wrote:
> jfb wrote:
> > @MadCoder does this size limit make sense to you, or would you recommend something smaller?
> 64k is wayyyy too large.
> 
> a kernel stack is 16k.
> 
> and we've seen that cause explosions in ioctl() kind of syscalls that have a large switch going in their various implementations where one of the functions has a large stack buffer and is a leaf and another path has no stack allocations but will go in a deep stack.
> 
> the threshold for the kernel is likely something like 1 or 2k
Nit: if this stays 65536, should it be typed uint32_t?? unsigned's size may not always be 4 bytes


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1936
 
+      auto AllocIsOversize = [&AI, DL]() {
+        auto Size = AI->getAllocationSizeInBits(DL);
----------------
Nit: IsAllocOverSizeLimit (or some other verb-starting alternative)


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1939
+        if (!Size)
+          return false;
+        return Size.getValue() / 8 > InlineConstants::MaxStaticAllocaSizeToMove;
----------------
aemerson wrote:
> jfb wrote:
> > This will also prevent allocations of unknown size from inlining, right? Is that a pessimization? I guess the problem you have is that you don't know whether you'll then propagate a huge size into the alloca, and then hoist it outside a block?
> > 
> > Would it make sense to both:
> > 
> > * Refuse inlining large alloca of known size
> > * Refuse moving large alloca of known size outside blocks
> > * Refuse moving variable-sized alloca (unknown size) outside blocks
> > 
> > ?
> > 
> > That way you can still inline functions with alloca of unknown size, you can still constant propagate into them, but you would hoist the alloca if it's big.
> This patch doesn't actually change any inlining heuristics. At this point we've already inlined the function, we're just deciding whether or not to move static allocas in the inlined body to the caller's entry block.
I think jfb's point is about the effects of that decision. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81765