| Message ID | 20151217211813.GB47204@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
> 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 <hubicka@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 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. 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. Thanks, Richard
> > 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 <hubicka@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. > > 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? 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. Honza > > Thanks, > Richard
Jan Hubicka <hubicka@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 <hubicka@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
> > 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. Ah, OK, now I get your point :) Yep, I have no problem beling conservative for non-anchor cases !SYMBOL_REF_DECL case. Pretty much all cases that matter are IMO either anchors or SYMBOL_REF_DECL != NULL. (i.e. user variables). I will update the patch and also look into the Alpha AND issues. Honza > > Thanks, > Richard
Index: alias.c =================================================================== --- alias.c (revision 231722) +++ alias.c (working copy) @@ -158,6 +158,7 @@ static int write_dependence_p (const_rtx, const_rtx, machine_mode, rtx, bool, bool, bool); +static int compare_base_symbol_refs (const_rtx, const_rtx); static void memory_modified_1 (rtx, const_rtx, void *); @@ -1756,16 +1757,8 @@ return LABEL_REF_LABEL (x) == LABEL_REF_LABEL (y); case SYMBOL_REF: - { - tree x_decl = SYMBOL_REF_DECL (x); - tree y_decl = SYMBOL_REF_DECL (y); + return compare_base_symbol_refs (x, y) == 1; - if (!x_decl || !y_decl) - return XSTR (x, 0) == XSTR (y, 0); - else - return compare_base_decls (x_decl, y_decl) == 1; - } - case ENTRY_VALUE: /* This is magic, don't go through canonicalization et al. */ return rtx_equal_p (ENTRY_VALUE_EXP (x), ENTRY_VALUE_EXP (y)); @@ -2052,6 +2045,65 @@ return ret; } +/* Same as compare_base_decls but for SYMBOL_REF. + Return -2 if the offset based oracle can not be used (i.e. + we have a symbol and section anchor which is located insite + the same block. */ + +static int +compare_base_symbol_refs (const_rtx x_base, const_rtx y_base) +{ + tree x_decl = SYMBOL_REF_DECL (x_base); + tree y_decl = SYMBOL_REF_DECL (y_base); + bool binds_def = true; + if (x_decl && y_decl) + return compare_base_decls (x_decl, y_decl); + if (x_decl || y_decl) + { + if (!x_decl) + { + std::swap (x_decl, y_decl); + std::swap (x_base, y_base); + } + /* Variable and anchor representing the variable alias. If x_base + is not a static variable or y_base is not an anchor (it is a label) + we are safe. */ + if (!SYMBOL_REF_HAS_BLOCK_INFO_P (y_base) + || (TREE_CODE (x_decl) != VAR_DECL + || (!TREE_STATIC (x_decl) && !TREE_PUBLIC (x_decl)))) + return 0; + symtab_node *x_node = symtab_node::get_create (x_decl) + ->ultimate_alias_target (); + /* External variable can not be in section anchor. */ + if (!x_node->definition) + return 0; + x_base = XEXP (DECL_RTL (x_node->decl), 0); + /* If not in anchor, we can disambiguate. */ + if (!SYMBOL_REF_HAS_BLOCK_INFO_P (x_base)) + return 0; + + /* We have an alias of anchored variable. If it can be interposed; + we must assume it may or may not alias its anchor. */ + binds_def = decl_binds_to_current_def_p (x_decl); + } + /* If we have variable in section anchor, we can compare by offset. */ + if (SYMBOL_REF_HAS_BLOCK_INFO_P (x_base) + && SYMBOL_REF_HAS_BLOCK_INFO_P (y_base)) + { + if (SYMBOL_REF_BLOCK (x_base) != SYMBOL_REF_BLOCK (y_base)) + return 0; + if (SYMBOL_REF_BLOCK_OFFSET (x_base) == SYMBOL_REF_BLOCK_OFFSET (y_base)) + return binds_def ? 1 : -1; + /* Mixing anchors and non-anchors may result to false negative. + We probably never do that. */ + if (SYMBOL_REF_ANCHOR_P (x_base) != SYMBOL_REF_ANCHOR_P (y_base)) + return -2; + return 0; + } + /* Label and label or label and section anchor. Copare symbol name. */ + return XSTR (x_base, 0) == XSTR (y_base, 0); +} + /* Return 0 if the addresses X and Y are known to point to different objects, 1 if they might be pointers to the same object. */ @@ -2090,16 +2142,8 @@ return 1; if (GET_CODE (x_base) == SYMBOL_REF && GET_CODE (y_base) == SYMBOL_REF) - { - tree x_decl = SYMBOL_REF_DECL (x_base); - tree y_decl = SYMBOL_REF_DECL (y_base); + return compare_base_symbol_refs (x_base, y_base) != 0; - /* We can assume that no stores are made to labels. */ - if (!x_decl || !y_decl) - return 0; - return compare_base_decls (x_decl, y_decl) != 0; - } - /* The base addresses are different expressions. If they are not accessed via AND, there is no conflict. We can bring knowledge of object alignment into play here. For example, on alpha, "char a, b;" can @@ -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); - } - else - cmp = compare_base_decls (x_decl, y_decl); - /* If both decls are the same, decide by offsets. */ if (cmp == 1) return offset_overlap_p (c, xsize, ysize); /* If decls are different or we know by offsets that there is no overlap, we win. */ - if (!cmp || !offset_overlap_p (c, xsize, ysize)) + if (!cmp || (cmp != -2 && !offset_overlap_p (c, xsize, ysize))) return 0; /* Decls may or may not be different and offsets overlap....*/ return -1;