Re: predicate aware uninitialized variable analysis

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Wed, Apr 21, 2010 at 5:43 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Wed, Apr 14, 2010 at 3:44 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Tue, Apr 13, 2010 at 9:23 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> The patch did not make into 4.5. Try it again for 4.6.
>>>
>>> There are new changes since the last attempt: The candidate list of
>>> PHIs (which may produce undefined values) are not precomputed. In the
>>> new patch, the predicate check will also be done on uses by phi
>>> arguments. Only when the uses ?in the phi arguments are not protected
>>> properly by a predicate, the phi statement will then be added to the
>>> worklist (and uses of its definitions will be checked later).
>>>
>>> Bootstrap and tested on x86_64/linux, and lots of apps.
>>
>> Can you instead of returning a sbitmap from compute_uninit_opnds_pos
>> use an unsigned int mask (with proper checks for PHIs with too many
>> operands which you don't want to handle because of complexity
>> reasons anyway)?
>>
>
> Done. Did not bother to check when there are more operands that can be handled.
>
>
>> + ?bool is_postdom = false;
>> +
>> + ?is_postdom = dominated_by_p (CDI_POST_DOMINATORS, bb2, bb1);
>> +
>> + ?if (!is_postdom)
>> + ? ?return false;
>>
>> save us code and simply write
>>
>> ?if (!dominated_by_p (CDI_POST_DOMINATORS, bb2, bb1))
>> ? return false;
>>
>
> Done.
>
>> You do compute control-dependences of some sort - can this
>> code be shared with the control-dependence computation in
>> tree-ssa-dce.c? ?Thus, can you re-use that instead? ?There's
>> the same find_pdom stuff you have as well.
>
> This is slightly different -- the one in this patch does control
> dependence computation on demand for bb of interests, which is more
> suitable.
>
>
>>
>> is_value_included_in looks like it should use code from tree-vrp.c.
>>
>
> Right, ideally the code should be shared. However there is no direct
> code that can be shared in the current implementation -- the code in
> value_inside_range is minimal, the majority of the code would be
> converting the range in uninit check to value_range_t form which still
> need to be written.
>
> However, it may be worth to refactor vrp code to be some common
> utility functions some day -- probably not in this patch.
>
>> It's always a good opportunity to separate and abstract out code
>> we can use from multiple places instead of re-inventing the
>> wheel over and over.
>
> Agree.
>
>
>>
>> find_matching_predicate_in_rest_chains also looks remotely
>> similar to what tree-ssa-loop-niter.c:simplify_using_initial_conditions
>> does.
>
> Similar to the vrp case -- some refactoring is needed before sharing can happen.
>
>>
>> The pass is rather large, so I appreciate some review from other
>> folks (I do believe it should try to use/enhance existing functionality).
>>
>
> Diego has done it internally.
>
>> Btw, thanks for the large collection of testcases. ?Can you check
>> which uninitialized vars bugs this fixes and add them to the
>> changelog entry?
>>
>
> Have checked some bug reports. Will update ?the change log when submitting it.
>
> Ok for check in?
>

This triggered:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45972


-- 
H.J.