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

[Amara Emerson via Phabricator via llvm-commits]

aemerson marked an inline comment as done.
aemerson added a comment.

In D81765#2090965 <https://reviews.llvm.org/D81765#2090965>, @efriedma wrote:

> Independent of the thresholds, I'd be very cautious about turning a static alloca into a dynamic alloca.  Our code generation isn't very sophisticated in a lot of cases; it often requires a "base pointer" to be generated in a fixed register.  This leads to a issues even if the alloca never runs: there's a performance penalty due to allocating the base pointer, and we can potentially break inline asm. We don't want to be doing that; if it comes down to either emitting a dynamic alloca or refusing to inline, refusing to inline is probably better.


How would generating a dynamic alloca break inline asm? That seems very fragile to me if that's the case.

64k was chosen, somewhat arbitrarily, as a value which no reasonable piece of code should be allocating more of in the stack. This isn't really intended to catch cases except in very rare circumstances where we get something like -1 as the alloc size. That said, I can look at changing this approach to avoid inlining large static allocas completely if we know that it would likely end up being a dynamic alloca in the caller.



================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1939
+        if (!Size)
+          return false;
+        return Size.getValue() / 8 > InlineConstants::MaxStaticAllocaSizeToMove;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81765