Re: [PATCH] Fix 70199

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Mar 15, 2016 at 8:42 PM, Richard Henderson <rth@redhat.com> wrote:
> On 03/15/2016 07:13 AM, Richard Biener wrote:
>>
>> On Tue, Mar 15, 2016 at 4:44 AM, Richard Henderson <rth@redhat.com> wrote:
>>>
>>> The problem here is that
>>>
>>>    void* labels[] = {
>>>      &&l0, &&l1, &&l2
>>>    };
>>>
>>> gets gimplified to
>>>
>>>    labels = *.LC0;
>>>
>>> but .LC0 is not in the set of local decls, so that when copy_forbidden is
>>> called during sra versioning we fail to forbid the copy.  We could set a
>>> different flag, but I think it's easiest to just add the artificial decl
>>> to
>>> where it can be seen.
>>>
>>> Ok?
>>
>>
>> Hmm.  tree_output_constant_def uses the global constant pool (and not
>> function-scope statics).  So while for the above case with local labels
>> there can be no sharing and thus the decl is really "local" with non-local
>> labels or with other random initializers you'd have the ctor decl in
>> multiple local decl vectors.  Not sure if that's a problem, but at least
>> if you'd have
>>
>>    void* labels[] = {
>>      &&l0, &&l1, &&l2
>>    };
>>    void* labels2[] = {
>>      &&l0, &&l1, &&l2
>>    };
>>
>> you'll end up with the same constant pool decl in local-decls twice.
>
>
> Yeah, but since the decl is TREE_STATIC, we'll ignore it for almost
> everything.  About the only thing I can figure that might go wrong is unused
> variable removal, where we'd remove the first copy but not look for
> duplicates, and so the variable stays in use when it isn't.  I don't *think*
> that can cause further problems.  It's not like we ever clear FORCED_LABEL
> even if the data referencing it goes away.
>
>> It's also
>> a bit pre-mature in the gimplifier as we only add to local-decls during
>> BIND expr lowering.
>
>
> Yeah, I suppose.  Though for a TREE_STATIC decl it doesn't make a difference
> that we didn't put it into any BIND_EXPR.
>
>> I also wonder if outputting the constant pool decl far away from the
>> labels
>> might end up with invalid asm for some targets.
>
>
> No.  The pointers involved here are full address space, not reduced
> displacement pc-relative.
>
>> Well, I don't see any convenient way of fixing things here either but
>> maybe
>> we can do
>>
>>    if (walk_tree_without_duplicataes (&DECL_INITIAL (ctor),
>> has_label_address_in_static_1, cfun->decl))
>>      add_local_decl (cfun, ctor);
>>
>> to avoid adding the decl when it is not necessary.
>
>
> Sure.  Patch 1 below.
>
>> Having another struct function flag would be possible as well, or re-use
>> has_nonlocal_label as clearly a global static is now refering to a local
>> label (you'd lose optimization when 'labels' becomes unused of course).
>
>
> On the other hand, the likelyhood of these labels (or the data referencing
> the labels) going away is slim.  Except for artificial test cases, the user
> is going to have taken these addresses and put them in an array for a
> reason.  The likelyhood of some stored FORCED_LABEL becoming non-forced is
> virtually nil.
>
> Patch 2 below.  This second patch does have lower complexity, and doesn't
> have the duplicated entry issue you point out.

I like patch 2 more - btw, you need to add has_forced_label_in_static streaming
to lto-streamer-{in,out}.c, just look for has_nonlocal_label streaming.  Also
has_label_address_in_static_1 is now unused and should be removed.

Thanks,
Richard.

> Thoughts?
>
>
> r~