[PATCH] D85848: [InlineCost] Fix scalable vectors in visitAlloca

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes marked an inline comment as done.
c-rhodes added inline comments.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:857
+  if (isa<ScalableVectorType>(I.getAllocatedType()))
+    return false;
+
----------------
efriedma wrote:
> c-rhodes wrote:
> > efriedma wrote:
> > > This will refuse to inline any function with a scalable vector variable; that seems so conservative that it's likely to cause practical issues.  Please take the time to fix the code properly.
> > > This will refuse to inline any function with a scalable vector variable; that seems so conservative that it's likely to cause practical issues. Please take the time to fix the code properly.
> > 
> > Is that not better than crashing? As far as I can tell either `getFixedSize` will be called causing a crash or it's a dynamic alloca that wont be inlined anyway.
> > 
> > I know we want to support scalable vectors here but unless I'm missing something this seems like a worthwhile improvement for the time being.
> Reading the code, I think you could just replace the uses of getFixedSize() with getKnownMinSize() and get roughly the right behavior.
> 
> The reason I don't want to put this off is that inlining is very important for optimizations; if we add a hack now, we're inevitably going to have to look at it again very soon, so I'd rather just get it done now.
>The reason I don't want to put this off is that inlining is very important for optimizations; if we add a hack now, we're inevitably going to have to look at it again very soon, so I'd rather just get it done now.

No worries, makes sense if it's as trivial as using `getKnownMinSize` instead


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

https://reviews.llvm.org/D85848