[PATCH] PR15967 basicaa claims no alias when there is

Karl-Johan Karlsson karl-johan.karlsson at ericsson.com
Wed Mar 26 06:02:49 PDT 2014


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
>>>
>>
>




More information about the llvm-commits mailing list