Hi,
On 20 November 2016 at 17:36, Aldy Hernandez <al...@redhat.com> 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 <al...@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?
>
>
Since this commit (r242639), I've noticed regressions on arm targets:
- PASS now FAIL [PASS => FAIL]:
gcc.dg/uninit-pred-6_a.c warning (test for warnings, line 36)
gcc.dg/uninit-pred-6_b.c warning (test for warnings, line 42)
gcc.dg/uninit-pred-7_c.c (test for excess errors)
gcc.dg/uninit-pred-7_c.c warning (test for warnings, line 29)
- FAIL appears [ => FAIL]:
gcc.dg/uninit-pred-7_c.c (internal compiler error)
This ICE says:
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/uninit-pred-7_c.c:
In function 'foo':
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/uninit-pred-7_c.c:9:5:
internal compiler error: in operator[], at vec.h:732
0xd64099 vec<pred_info, va_heap, vl_embed>::operator[](unsigned int)
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/vec.h:732
0xd64099 vec<pred_info, va_heap, vl_ptr>::operator[](unsigned int)
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/vec.h:1216
0xd64099 can_chain_union_be_invalidated_p
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree-ssa-uninit.c:2195
0xd64099 uninit_ops_invalidate_phi_use
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree-ssa-uninit.c:2301
0xd649d0 is_use_properly_guarded
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree-ssa-uninit.c:2365
0xd65b00 find_uninit_use
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree-ssa-uninit.c:2434
0xd65b00 warn_uninitialized_phi
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree-ssa-uninit.c:2504
0xd65b00 execute
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree-ssa-uninit.c:2612
In this case, GCC was configured with:
target arm-none-linux-gnueabihf
--with-cpu=cortex-a5 --with-fpu=vfpv3-d16-fp16
The other CPUs I check work fine (a9/a15/a57).
Christophe
