Re: predicate aware uninitialized variable analysis

[Xinliang David Li <davidxl at google dot com>]

Please review the patch attached. The number of operand of the phi
exceeds the width of the bit vector (fixed length for efficiency)
leading to the ICE later.

OK to checkin after test is done?

Thanks,

David

On Mon, Oct 11, 2010 at 5:09 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> 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.
>

Attachment: phi_ovf.p
Description: Binary data

Attachment: cl
Description: Binary data