Patch Review for PR17158

[Bill Wendling]

It does look like we're trying to solve the same problem. Yours might be a more general solution than mine. I commented on your patch. I still want to consult with Chandler to make sure that it's the correct decision. :-)

-bw

On Nov 6, 2013, at 4:36 PM, David Majnemer <david.majnemer at gmail.com> wrote:

> This looks awfully familiar to a bug that my patch fixes.  I tried a similar approach earlier in http://llvm-reviews.chandlerc.com/D2060 but instead settled on http://llvm-reviews.chandlerc.com/D2093
> 
> I have a strong feeling that my second patch will fix your bug (and potentially others).
> 
>  -- 
> David Majnemer
> 
> 
> On Wed, Nov 6, 2013 at 4:07 PM, Bill Wendling <isanbard at gmail.com> wrote:
> Hi Chandler et al,
> 
> This is a potential patch for PR17158. What seems to be happening is that GVN gets ahold of the file, it tries to determine if the initializer for x7 can be folded into the use. Because of SROA, it's doing some weird pointer things involving the structure padding. So for instance, the structure is this:
> 
>         { { i32, i8, [3 x i8] }, i8, [3 x i8] }
> 
> It copies over the 'i32' and 'i8' from the inner structure. SROA then generates a memcpy from the [3 x i8] in the inner structure to the end of the outer structure, which sounds okay. However, GVN looks at the access to the outer 'i8' element, but because of SROA, that element is generated as the element of the inner [3 x i8] at offset 3. This is of course outside of the array. Because it's padding, the value it returns is undef. Thus it folds the value of '0' into the call to 'printf'.
> 
> Got that so far? ;-)
> 
> Here's a patch I have. It basically makes sure that, if the constant it's looking at is an array, the index into that array does not go outside of that array. If that happens, then there's something complex going on and the constant folding isn't safe anymore.
> 
> What do you think? Is this the correct solution?
> 
> -bw
> 
> Run the attached t.ll with "opt -sroa -instcombine -gvn", generate the .s file, and run it. It should print '1', but instead prints '0'.
> 
> diff --git a/lib/Analysis/ConstantFolding.cpp b/lib/Analysis/ConstantFolding.cpp
> index e093631..527fa80 100644
> --- a/lib/Analysis/ConstantFolding.cpp
> +++ b/lib/Analysis/ConstantFolding.cpp
> @@ -1145,7 +1145,11 @@ Constant *llvm::ConstantFoldLoadThroughGEPConstantExpr(Constant *C,
>    // Loop over all of the operands, tracking down which value we are
>    // addressing.
>    for (unsigned i = 2, e = CE->getNumOperands(); i != e; ++i) {
> -    C = C->getAggregateElement(CE->getOperand(i));
> +    Constant *Offset = CE->getOperand(i);
> +    if (const ArrayType *AT = dyn_cast<ArrayType>(C->getType()))
> +      if (AT->getNumElements() <= cast<ConstantInt>(Offset)->getZExtValue())
> +        return 0;
> +    C = C->getAggregateElement(Offset);
>      if (C == 0)
>        return 0;
>    }
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131106/e32c7002/attachment.html>