PR61409: -Wmaybe-uninitialized false-positive with -O2

[Jeff Law]

On 11/20/2016 09:36 AM, Aldy Hernandez wrote:
> On 11/16/2016 03:57 PM, Jeff Law wrote:
>> On 11/02/2016 11:16 AM, Aldy Hernandez wrote:
>>> Hi Jeff.
>>>
>>> As discussed in the PR, here is a patch exploring your idea of ignoring
>>> unguarded uses if we can prove that the guards for such uses are
>>> invalidated by the uninitialized operand paths being executed.
>>>
>>> This is an updated patch from my suggestion in the PR.  It bootstraps
>>> with no regression on x86-64 Linux, and fixes the PR in question.
>>>
>>> As the "NOTE:" in the code states, we could be much smarter when
>>> invalidating predicates, but for now let's do straight negation which
>>> works for the simple case.  We could expand on this in the future.
>>>
>>> OK for trunk?
>>>
>>>
>>> curr
>>>
>>>
>>> commit 8375d7e28c1a798dd0cc0f487d7fa1068d9eb124
>>> Author: Aldy Hernandez <aldyh@redhat.com>
>>> Date:   Thu Aug 25 10:44:29 2016 -0400
>>>
>>>         PR middle-end/61409
>>>         * tree-ssa-uninit.c (use_pred_not_overlap_with_undef_path_pred):
>>>         Remove reference to missing NUM_PREDS in function comment.
>>>         (can_one_predicate_be_invalidated_p): New.
>>>         (can_chain_union_be_invalidated_p): New.
>>>         (flatten_out_predicate_chains): New.
>>>         (uninit_ops_invalidate_phi_use): New.
>>>         (is_use_properly_guarded): Call uninit_ops_invalidate_phi_use.
>> [ snip ]
>>
>>>
>>> +static bool
>>> +can_one_predicate_be_invalidated_p (pred_info predicate,
>>> +                    vec<pred_info *> worklist)
>>> +{
>>> +  for (size_t i = 0; i < worklist.length (); ++i)
>>> +    {
>>> +      pred_info *p = worklist[i];
>>> +
>>> +      /* NOTE: This is a very simple check, and only understands an
>>> +     exact opposite.  So, [i == 0] is currently only invalidated
>>> +     by [.NOT. i == 0] or [i != 0].  Ideally we should also
>>> +     invalidate with say [i > 5] or [i == 8].  There is certainly
>>> +     room for improvement here.  */
>>> +      if (pred_neg_p (predicate, *p))
>> It's good enough for now.  I saw some other routines that might allow us
>> to handle more cases.  I'm OK with faulting those in if/when we see such
>> cases in real code.
>
> Ok.
>
>>
>>
>>> +
>>> +/* Return TRUE if executing the path to some uninitialized operands in
>>> +   a PHI will invalidate the use of the PHI result later on.
>>> +
>>> +   UNINIT_OPNDS is a bit vector specifying which PHI arguments have
>>> +   arguments which are considered uninitialized.
>>> +
>>> +   USE_PREDS is the pred_chain_union specifying the guard conditions
>>> +   for the use of the PHI result.
>>> +
>>> +   What we want to do is disprove each of the guards in the factors of
>>> +   the USE_PREDS.  So if we have:
>>> +
>>> +   # USE_PREDS guards of:
>>> +   #    1. i > 5 && i < 100
>>> +   #    2. j > 10 && j < 88
>> Are USE_PREDS joined by an AND or IOR?  I guess based on their type it
>> must be IOR.   Thus to get to a use  #1 or #2 must be true.  So to prove
>> we can't reach a use, we have to prove that #1 and #2 are both not true.
>>  Right?
>
> IOR, so yes.
>
>>
>>
>>> +
>>> +static bool
>>> +uninit_ops_invalidate_phi_use (gphi *phi, unsigned uninit_opnds,
>>> +                   pred_chain_union use_preds)
>>> +{
>>> +  /* Look for the control dependencies of all the uninitialized
>>> +     operands and build predicates describing them.  */
>>> +  unsigned i;
>>> +  pred_chain_union uninit_preds[32];
>>> +  memset (uninit_preds, 0, sizeof (pred_chain_union) * 32);
>>> +  for (i = 0; i < MIN (32, gimple_phi_num_args (phi)); i++)
>> Can you replace the magic "32" with a file scoped const or #define?  I
>> believe there's 2 existing uses of a magic "32" elsewhere in
>> tree-ssa-uninit.c as well.
>
> Done.
>
>>
>>> +
>>> +      /* Build the control dependency chain for `i'...  */
>>> +      if (compute_control_dep_chain (find_dom (e->src),
>>> +                     e->src,
>>> +                     dep_chains,
>>> +                     &num_chains,
>>> +                     &cur_chain,
>>> +                     &num_calls))
>> Does this miss the control dependency carried by E itself.
>>
>> ie, if e->src ends in a conditional, shouldn't that conditional be
>> reflected in the control dependency chain as well?  I guess we'd have to
>> have the notion of computing the control dependency for an edge rather
>> than a block.  It doesn't look like compute_control_dep_chain is ready
>> for that.  I'm willing to put that into a "future work" bucket.
>
> Hmmm, probably not.  So yeah, let's put that in the future work bucket :).
>
>>
>> So I think just confirming my question about how USE_PREDS are joined at
>> the call to uninit_opts_invalidate_phi_use and fixing the magic 32 to be
>> a file scoped const or a #define and this is good to go on the trunk.
>
> I've retested with no regressions on x86-64 Linux.
>
> OK for trunk?
Yes.  Thanks for taking my vague idea and making it real :-)

I've got another if you're interested...

jeff