[MemoryBuiltins] Allow truncation in visitAllocaInst()

[uabelho]

Summary

Solves PR33689.

If the pointer size is less than the size of the type used for the array
size in an alloca (the <ty> type below) then we could trigger the assert in
the PR. In that example we have pointer size i16 and <ty> is i32.

<result> = alloca [inalloca] <type> [, <ty> <NumElements>] [, align <alignment>]

Handle the situation by allowing truncation as well as zero extension in
ObjectSizeOffsetVisitor::visitAllocaInst().

Also, we now detect overflow in visitAllocaInst(), similar to how it was
already done in visitCallSite().

[uabelho]

If the truncation actually truncates something (and not just changes e.g. an i32 1 to an i16 1)
then we are not in a good spot, but then I suppose we have already messed up since then we
would have something like

%foo = alloca i16 , i32 1000000

with pointers being 16b wide so the allocated temporary wouldn't fit in memory
anyway.

I don't know this code very well though so if you think there is a more reasonable fix for this
please let me know.

[george.burgess.iv]

Thanks for this! One nit, and I think it looks good.

lib/Analysis/MemoryBuiltins.cpp

518

Addressing your comment: I realize that this is an existing inconsistency, but we try to return unknown() if an overflow happens (e.g. on line 597). While we're in the area, can we make this test for overflow too, please?

[uabelho]

Thanks for the comments!

lib/Analysis/MemoryBuiltins.cpp

518

So you want me to detect if overflow actually happens and if so return unknown(), and add a testcase for the overflow case as well?

If so, yes I'll try.

[george.burgess.iv]

lib/Analysis/MemoryBuiltins.cpp

518

Yes, please.

I'm not 100% sure if testing this will be straightforward, though. If it seems nontrivial, just let me know. Since it's my nit, I'm happy to try to make a test for overflow and commit it after this lands. :)

[uabelho]

lib/Analysis/MemoryBuiltins.cpp

518

Ok I will!

Yes the testing I don't know since I think the test case I have already is pretty non-straightforward.. The i64 is being converted to a [2 x i32] and I think it's that "2" that I will truncate.

So for overflow to happen with just small modifications of my existing test I'll need my %i64_t to be something huge. I'll give it a try though.

[uabelho]

Added overflow checks to visitAllocaInst.

To avoid some code duplication I pulled out the CheckedZextOrTrunc lambda from visitCallSite
and made a member function of it.

[uabelho]

lib/Analysis/MemoryBuiltins.cpp

518

Added overflow checks. Something like this is what you had in mind?

For the testcase, if I change
%i64_t = type i64
to
%i64_t = type i6488888
then the overflow checks trigger, but the end result from instcombine is still the same
as without overflow checks so I don't have any nice output change to check for.

If you know this code then I suppose you'll be able to make a testcase for it.

[george.burgess.iv]

LGTM after a few stylistic nits are addressed. Thanks again!

lib/Analysis/MemoryBuiltins.cpp

511

I think it's \p I, though I'm admittedly not great with doxygen.

518

Something like this is what you had in mind?

Yup!

If you know this code then I suppose you'll be able to make a testcase for it

Will do. :)

520

Please remove the brackets, since this is a single-statement if

522

Please remove the trailing ;

[uabelho]

Addressed the few remaining issues.

(Totally idiotic to not see them myself to begin with. Sorry and thanks!)

Updated the summary so it now has a few words about the new overflow
checks in visitAllocaInst().

[uabelho]

Thanks!

[george.burgess.iv]

LGTM!

Totally idiotic to not see them myself to begin with. Sorry and thanks!

Not a problem :)

[uabelho]

Apparently I accidentally dropped the test case between two revisions of this patch so I submitted it
now in https://reviews.llvm.org/rL307887

lib/Analysis/MemoryBuiltins.cpp

SizeOffsetType ObjectSizeOffsetVisitor::compute(Value *V) {
   DEBUG(dbgs() << "ObjectSizeOffsetVisitor::compute() unhandled value: " << *V
         << '\n');
   return unknown();
 }
 
 SizeOffsetType ObjectSizeOffsetVisitor::visitAllocaInst(AllocaInst &I) {
   if (!I.getAllocatedType()->isSized())
     return unknown();
 
   APInt Size(IntTyBits, DL.getTypeAllocSize(I.getAllocatedType()));
   if (!I.isArrayAllocation())
     return std::make_pair(align(Size, I.getAlignment()), Zero);
 
   Value *ArraySize = I.getArraySize();
   if (const ConstantInt *C = dyn_cast<ConstantInt>(ArraySize)) {
-    Size *= C->getValue().zextOrSelf(IntTyBits);
+    Size *= C->getValue().zextOrTrunc(IntTyBits);
     return std::make_pair(align(Size, I.getAlignment()), Zero);
   }
   return unknown();
 }
 
 SizeOffsetType ObjectSizeOffsetVisitor::visitArgument(Argument &A) {
   // No interprocedural analysis is done at the moment.
   if (!A.hasByValOrInAllocaAttr()) {
     ++ObjectVisitorArgument;
     return unknown();
   }
   PointerType *PT = cast<PointerType>(A.getType());

test/Transforms/InstCombine/pr33689_same_bitwidth.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -instcombine %s -o - | FileCheck %s
+
+; All the "useless" instructions should be removed and we shouldn't crash.
+
+target datalayout = "p:16:16"
+
+%i64_t = type i64
+
+@a = external global i16
+@b = external global i16*
+
+define void @f() {
+; CHECK-LABEL: @f(
+; CHECK-NEXT:  bb0:
+; CHECK-NEXT:    [[TMP12:%.*]] = alloca [2 x i32], align 8
+; CHECK-NEXT:    [[TMP12_SUB:%.*]] = getelementptr inbounds [2 x i32], [2 x i32]* [[TMP12]], i16 0, i16 0
+; CHECK-NEXT:    br i1 undef, label [[BB1:%.*]], label [[BB2:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[TMP8:%.*]] = ptrtoint [2 x i32]* [[TMP12]] to i16
+; CHECK-NEXT:    store i16 [[TMP8]], i16* @a, align 2
+; CHECK-NEXT:    unreachable
+; CHECK:       bb2:
+; CHECK-NEXT:    [[TMP9:%.*]] = load i16*, i16** @b, align 2
+; CHECK-NEXT:    store i16 0, i16* [[TMP9]], align 2
+; CHECK-NEXT:    [[TMP10:%.*]] = load i32, i32* [[TMP12_SUB]], align 8
+; CHECK-NEXT:    [[TMP11:%.*]] = add i32 [[TMP10]], -1
+; CHECK-NEXT:    store i32 [[TMP11]], i32* [[TMP12_SUB]], align 8
+; CHECK-NEXT:    ret void
+;
+bb0:
+  %tmp1 = alloca %i64_t
+  %tmp2 = bitcast %i64_t* %tmp1 to i32*
+  %useless3 = bitcast %i64_t* %tmp1 to i16*
+  %useless4 = getelementptr inbounds i16, i16* %useless3, i16 undef
+  %useless5 = bitcast i16* %useless4 to i32*
+  br i1 undef, label %bb1, label %bb2
+
+bb1:                                              ; preds = %bb0
+  %useless6 = insertvalue [1 x i32*] undef, i32* %tmp2, 0
+  %useless7 = insertvalue [1 x i32*] %useless6, i32* null, 0
+  %tmp8 = ptrtoint i32* %tmp2 to i16
+  store i16 %tmp8, i16* @a
+  unreachable
+
+bb2:                                              ; preds = %bb0
+  %tmp9 = load i16*, i16** @b
+  store i16 0, i16* %tmp9
+  %tmp10 = load i32, i32* %tmp2
+  %tmp11 = sub i32 %tmp10, 1
+  store i32 %tmp11, i32* %tmp2
+  ret void
+}