[Inliner] don't assume that a Constant alloca size is a ConstantInt (PR27277)

[spatel]

Summary

Regarding the FIXME: no tests fail if I remove the 'AllocatedSize' line completely.

[mehdi_amini]

lib/Analysis/InlineCost.cpp

335

We're losing the assertion, that could be seen as somehow not totally useless: if the only special case is undef, maybe it could be spelled out explicitly?
(I'm not having a strong opinion either way)

339

Indeed...

My impression is that this is the kind of things that can only be reliably tested using a c++ unitest. I doubt that InlineCost has much unittest coverage though.

[spatel]

lib/Analysis/InlineCost.cpp

335

I've never looked at alloca or this file before, but that seems reasonable (it can't be FP or vector). Let me update the patch.

[spatel]

Patch updated:
Explicitly check for an undef and keep the assert.

[mehdi_amini]

LGTM.

Do you plan to act on the FIXME?

[majnemer]

lib/Analysis/InlineCost.cpp

335

This is wrong. This could will crash if it is a ConstantExpr.

[spatel]

In D20077#425070, @joker.eph wrote:

LGTM.

Thanks!

Do you plan to act on the FIXME?

I would if I knew where the test should go. If you want to handle that, I'll watch and learn. :)

[davidxl]

lgtm

[davidxl]

Obviously David M caught a problem ..

[spatel]

Patch updated:
Back to the original version - if it's not a ConstantInt, we ignore it.

[mehdi_amini]

There should be a test-case for the problem David mentioned.

[spatel]

Patch updated:
Add a ConstExpr test case.

[majnemer]

I'd prefer auto * in the if statement

[spatel]

Patch updated:
Use 'auto' with dyn_cast_or_null.
Also, keep the original 'Size' var because the 80-col forced formatting for the alternative is ugly.

[majnemer]

LGTM

lib/Analysis/InlineCost.cpp

bool CallAnalyzer::accumulateGEPOffset(GEPOperator &GEP, APInt &Offset) {
   }
   return true;
 }
 
 bool CallAnalyzer::visitAlloca(AllocaInst &I) {
   // Check whether inlining will turn a dynamic alloca into a static
   // alloca, and handle that case.
   if (I.isArrayAllocation()) {
-    if (Constant *Size = SimplifiedValues.lookup(I.getArraySize())) {
-      ConstantInt *AllocSize = dyn_cast<ConstantInt>(Size);
-      assert(AllocSize && "Allocation size not a constant int?");
+    Constant *Size = SimplifiedValues.lookup(I.getArraySize());
+    if (Size && !isa<UndefValue>(Size)) {
+      assert(isa<ConstantInt>(Size) && "Allocation size not a constant int?");
+      ConstantInt *AllocSize = cast<ConstantInt>(Size);
       Type *Ty = I.getAllocatedType();
+      // FIXME: This can't be right. AllocatedSize is in *bytes*.
       AllocatedSize += Ty->getPrimitiveSizeInBits() * AllocSize->getZExtValue();
       return Base::visitAlloca(I);
     }
   }
 
   // Accumulate the allocated size.
   if (I.isStaticAlloca()) {
     const DataLayout &DL = F.getParent()->getDataLayout();

test/Transforms/Inline/alloca_test.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; This test ensures that alloca instructions in the entry block for an inlined
 ; function are moved to the top of the function they are inlined into.
 ;
 ; RUN: opt -S -inline < %s | FileCheck %s
 
 define i32 @func(i32 %i) {
   %X = alloca i32
   store i32 %i, i32* %X

 ;
 Entry:
   call void @bar( )
   %X = call i32 @func( i32 7 )
   %Y = add i32 %X, %argc
   ret i32 %Y
 }
 
+; https://llvm.org/bugs/show_bug.cgi?id=27277
+; Don't assume that the size is a ConstantInt (an undef value is also considered a constant).
+
+define void @PR27277(i32 %p1) {
+; CHECK-LABEL: @PR27277(
+; CHECK-NEXT:    [[VLA:%.*]] = alloca double, i32 %p1
+; CHECK-NEXT:    call void @PR27277(i32 undef)
+; CHECK-NEXT:    ret void
+;
+  %vla = alloca double, i32 %p1
+  call void @PR27277(i32 undef)
+  ret void
+}
+