[PATCH] PR15967 basicaa claims no alias when there is

[Karl-Johan Karlsson]

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.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug_15967_basicaa.patch
Type: text/x-patch
Size: 9356 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140319/952b6862/attachment.bin>