[PATCH] PR15967 basicaa claims no alias when there is

[Arnold Schwaighofer]

Committed as r204859.

Thanks!

On Mar 26, 2014, at 6:02 AM, Karl-Johan Karlsson <karl-johan.karlsson at ericsson.com> wrote:

> On 2014-03-19 14:19, Karl-Johan Karlsson wrote:
>> Hi hfinkel,
>> 
>> 
>> On 2014-03-18 17:42, Hal Finkel wrote:
>>> Karl-Johan,
>>> 
>>> +// The max limit of the search depth in DecomposeGEPExpression() and
>>> 
>>> +// GetUnderlyingObject(), both functions need to use the same search
>>> 
>>> +// depth otherwise the algorithm assert false.
>>> 
>>> +static const unsigned MaxLookupSearchDepth = 6;
>>> 
>>> Okay, this is good, but:
>>> 
>>>  - "otherwise the algorithm assert false." -- This does not parse for
>>> me. Do you mean that it will assert, or do you mean that it may return
>>> an invalid answer. Given the PR, it seems like the latter.
>>> 
>>>  - Please add a *strong* warning, both in the function body of
>>> DecomposeGEPExpression and also in the body of
>>> llvm::GetUnderlyingObject warning that these two functions are
>>> semantically linked, and that llvm::GetUnderlyingObject must never
>>> gain the ability for recurse farther than DecomposeGEPExpression for
>>> any given depth limit.
>> 
>> Sorry for the confusion. What I mean is that if the search depth are not
>> the same, asserts will blow in the aliasGEP function. The reason for
>> creating the MaxLookupSearchDepth was to clarify that the search depth
>> have to be the same. Before, the code was relying on the the default
>> parameter of GetUnderlyingObject (set to 6) was the same as the
>> MaxLookup variable in DecomposeGEPExpression (also set to 6). Nothing
>> was wrong in this part of the code. This was merely a cleanup.
>> 
>> I think that DecomposeGEPExpression is semantically linked to
>> GetUnderlyingObject, therefor a note about this might be appropriate in
>> the function body of DecomposeGEPExpression.
>> 
>>> +        // If the max search depth is reached the result is undefined
>>> 
>>> +        if (GEP2MaxLookupReached || GEP1MaxLookupReached) {
>>> 
>>> +          return MayAlias;
>>> 
>>> +        }
>>> 
>>> don't add { } around single-line ifs like this. (see the code examples
>>> in http://llvm.org/docs/CodingStandards.html)
>> 
>> Sure, I will remove { } around single-lines.
>> 
>> I have added more describing text to the commit message in the new
>> attached patch.
> 
> As I haven't heard anything more about this patch I assume everyone is happy with the changes I made to the patch. Any more comments? Or can someone please commit the patch?
> 
> Regards
> / Karl-Johan Karlsson
> 
>> 
>> Thanks for reviewing this patch.
>> 
>> Regards
>> / Karl-Johan Karlsson
>> 
>>> 
>>> +    // If the max search depth is reached the result is undefined
>>> 
>>> +    if (GEP2MaxLookupReached || GEP1MaxLookupReached) {
>>> 
>>> +      return MayAlias;
>>> 
>>> +    }
>>> 
>>> same here
>>> 
>>> +    // If the max search depth is reached the result is undefined
>>> 
>>> +    if (GEP1MaxLookupReached) {
>>> 
>>> +      return MayAlias;
>>> 
>>> +    }
>>> 
>>> same here
>>> 
>>> Thanks for working on this!
>>> 
>>>  -Hal
>>> 
>>> ----- Original Message -----
>>>> From: "Karl-Johan Karlsson" <karl-johan.karlsson at ericsson.com>
>>>> To: llvm-commits at cs.uiuc.edu
>>>> Sent: Tuesday, March 18, 2014 9:14:14 AM
>>>> Subject: [PATCH] PR15967 basicaa claims no alias when there is
>>>> 
>>>> Hi,
>>>> 
>>>> Fix in basicaa for problem when the MaxLookup limit was reached.
>>>> 
>>>> / Karl-Johan Karlsson
>>>> 
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>> 
>>> 
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits