Patch Review for PR17158

[David Majnemer]

On Wed, Nov 6, 2013 at 11:07 PM, Bill Wendling <isanbard at gmail.com> wrote:

> 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. :-)
>

I've updated the patch to (hopefully) address your comments.  For the
record, my patch also fixes PR17158.


>
> -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/20131107/b13374fb/attachment.html>