[PATCH] D35003: [MemoryBuiltins] Allow truncation in visitAllocaInst()

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

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



================
Comment at: lib/Analysis/MemoryBuiltins.cpp:518
   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);
----------------
uabelho wrote:
> uabelho wrote:
> > george.burgess.iv wrote:
> > > uabelho wrote:
> > > > george.burgess.iv wrote:
> > > > > 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?
> > > > 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.
> > > 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. :)
> > 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.
> 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. 
> 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. :)


================
Comment at: lib/Analysis/MemoryBuiltins.cpp:511
+/// trouble with APInt size issues. This function handles resizing + overflow
+/// checks for us. Check and zext or trunc /p I depending on IntTyBits and
+/// I's value.
----------------
I think it's `\p I`, though I'm admittedly not great with doxygen.


================
Comment at: lib/Analysis/MemoryBuiltins.cpp:522
+  return true;
+};
+
----------------
Please remove the trailing `;`


================
Comment at: lib/Analysis/MemoryBuiltins.cpp:535
+    APInt NumElems = C->getValue();
+    if (!CheckedZextOrTrunc(NumElems)) {
+      return unknown();
----------------
Please remove the brackets, since this is a single-statement `if`


https://reviews.llvm.org/D35003