Jan Hubicka <hubi...@ucw.cz> writes:
>> > Hi,
>> > the alias-2.c testcase fails on targets with anchors. The reason is that
>> > the variable itself is anchored while the alias is not and they point to
>> > the
>> > same location. I folllowed the docs of SYMBOL_REF claiming that
>> > SYMBOL_REF_DECL if the symbol is label and tought it is safe to
>> > disambiguate
>> > them.
>>
>> What kind of distinction do you mean between "label" and non-label here?
>>
>> I don't think !SYMBOL_REF_DECL tells us anything useful about the
>> symbol. I think it's more a case of: if SYMBOF_REF_DECL is present,
>> we can assume that what it says is accurate. If it isn't present we
>> can't assume anything.
>>
>> So was it...
>>
>> Jan Hubicka <hubi...@ucw.cz> writes:
>> > @@ -2323,26 +2367,14 @@
>> >
>> > if (GET_CODE (x) == SYMBOL_REF && GET_CODE (y) == SYMBOL_REF)
>> > {
>> > - tree x_decl = SYMBOL_REF_DECL (x);
>> > - tree y_decl = SYMBOL_REF_DECL (y);
>> > - int cmp;
>> > + int cmp = compare_base_symbol_refs (x,y);
>> >
>> > - if (!x_decl || !y_decl)
>> > - {
>> > - /* Label and normal symbol are never the same. */
>> > - if (x_decl != y_decl)
>> > - return 0;
>> > - return offset_overlap_p (c, xsize, ysize);
>>
>> ...this part of the code that was causing the problem? That doesn't
>> seem valid even without the anchor stuff. I think the starting position
>
> Yep, I misread the docs assuming that SYMBOL_REF_DECL == NULL only
> for code labels. It is always safe to disambiguate these as they access
> readonly memory anyway.
>
>> should be that we can only use offset_overlap_p if XSTR (x) == XSTR (y)
>> and need to assume a conflict otherwise. E.g. I think in principle it's
>> valid for a target to create symbol_refs for different parts of a single
>> artificial object.
>
> The code original was:
> if (rtx_equal_for_memref_p (x, y))
> {
> return offset_overlap_p (c, xsize, ysize);
> }
> and
> rtx_equal_for_memref_p (const_rtx x, const_rtx y)
> {
> ...
> case SYMBOL_REF:
> return XSTR (x, 0) == XSTR (y, 0);
>
> So it assumed base+offset check for all labels with same XSTR. My patch
> makes this strictly weaker: it is possible that the same variable is
> accessed via its own symbol or via offset from variable anchor.
Well, to me "weaker" means "makes more conservative assumptions",
which in this context would be assuming a conflict in cases where
the old code didn't (i.e. returning -1 more often). I'm not sure
whether the first patch was strictly weaker in that sense, since
if the symbol_refs were not equal according to rtx_equal_for_memref_p,
the old code would fall through to the end of the function and return -1.
>> I agree there are other refinements you can do on top of that,
>> e.g. that two block symbols in different blocks can never conflict.
>> But the patch seems to be treating anchors as an exception to the rule,
>> whereas I think they're just one instance of the rule.
>
> Can you think of some testcase?
Not a specific one, sorry, but it seems like the kind of thing that
could happen with extra ABI-mandated constructs.
But maybe the lack of a specific testcase is a good thing. If in practice
anchors make up the vast majority of cases where (a) SYMBOL_REF_DECL
is null and (b) XSTR is too weak, there should no harm in relying on
the old XSTR comparison for the non-anchor null-decl cases.
> Doing XSTR==XSTR test and assuming a conflict otherwise will cause a
> conflict between
> every external variable read/write and static variable read/write as one
> will be anchored
> and other not.
Yeah, I think handling anchors is a good thing. It just seems that
logically the correctness fix is to replace:
/* Label and normal symbol are never the same. */
if (x_decl != y_decl)
return 0;
return offset_overlap_p (c, xsize, ysize);
with something like:
if (XSTR (x, 0) == XSTR (y, 0))
return offset_overlap_p (c, xsize, ysize);
/* Symbols might conflict. */
return -1;
Handling anchors would then be a very useful optimisation on top of that.
Thanks,
Richard
