| Message ID | CAFULd4ZBp4fubCh6ByezoS9AfjQk2zwQ_SdmRfb4Keo2ob3saw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 10/10/14 04:06, Uros Bizjak wrote: > On Wed, Oct 8, 2014 at 1:56 PM, Uros Bizjak <ubizjak@gmail.com> wrote: > >>>> This message revives an old thread [1], where the miscompilation of gfortran >>>> on alpha was found that that resulted in: >> >> [...] >> >>> As said in the audit trail of the bugreport I think that the caller >>> of alpha_set_memflags is wrong in applying MEM flags from the _source_ >>> operand to MEMs generated for the RMW sequence for the destination. >>> >>> It would be better to fix that instead. >> >> Please see comment #6 of the referred PR [1] for further analysis and >> ammended testcase. The testcase and analysis will show a "native" read >> passing possibly aliasing store. > > Attached v2 patch implements the same approach in all alias.c places > that declare MEM_READONLY_P operands as never aliasing. > > 2014-10-10 Uros Bizjak <ubizjak@gmail.com> > > * alias.c (true_dependence_1): Do not exit early for MEM_READONLY_P > references when alignment ANDs are involved. > (write_dependence_p): Ditto. > (may_alias_p): Ditto. > > Patch was boostrapped and regression tested on x86_64-linux-gnu and > alpha-linux-gnu. Unfortunately, there are still failures remaining in > gfortran testsuite due to independent RTL infrastructure problem with > VALUEs leaking into aliasing detecting functions [2], [3]. > > The patch was discussed and OK'd by Richi in the PR audit trail. If > there are no objections from RTL maintainers, I plan to commit it to > the mainline early next week. > > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63483 > [2] https://gcc.gnu.org/ml/gcc/2014-10/msg00060.html > [3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63475 No objection. In fact, after reading everything it's pretty obvious to me that a /u MEM must be considered as potentially conflicting with writes that are implemented as RMW sequences to deal with the lack of byte access support. The escaping VALUE stuff is still in my queue. jeff
On Fri, Oct 10, 2014 at 7:25 PM, Jeff Law <law@redhat.com> wrote: >>>>> This message revives an old thread [1], where the miscompilation of >>>>> gfortran >>>>> on alpha was found that that resulted in: >>> >>> >>> [...] >>> >>>> As said in the audit trail of the bugreport I think that the caller >>>> of alpha_set_memflags is wrong in applying MEM flags from the _source_ >>>> operand to MEMs generated for the RMW sequence for the destination. >>>> >>>> It would be better to fix that instead. >>> >>> >>> Please see comment #6 of the referred PR [1] for further analysis and >>> ammended testcase. The testcase and analysis will show a "native" read >>> passing possibly aliasing store. >> >> >> Attached v2 patch implements the same approach in all alias.c places >> that declare MEM_READONLY_P operands as never aliasing. >> >> 2014-10-10 Uros Bizjak <ubizjak@gmail.com> >> >> * alias.c (true_dependence_1): Do not exit early for MEM_READONLY_P >> references when alignment ANDs are involved. >> (write_dependence_p): Ditto. >> (may_alias_p): Ditto. >> >> Patch was boostrapped and regression tested on x86_64-linux-gnu and >> alpha-linux-gnu. Unfortunately, there are still failures remaining in >> gfortran testsuite due to independent RTL infrastructure problem with >> VALUEs leaking into aliasing detecting functions [2], [3]. >> >> The patch was discussed and OK'd by Richi in the PR audit trail. If >> there are no objections from RTL maintainers, I plan to commit it to >> the mainline early next week. >> >> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63483 >> [2] https://gcc.gnu.org/ml/gcc/2014-10/msg00060.html >> [3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63475 > > No objection. In fact, after reading everything it's pretty obvious to me > that a /u MEM must be considered as potentially conflicting with writes that > are implemented as RMW sequences to deal with the lack of byte access > support. Thanks, I went ahead and commit the patch to SVN mainline. I wonder, if they should be also committed to release branches? > The escaping VALUE stuff is still in my queue. Great, I can test them on alpha native, there are many gfortran testsuite failures due to this problem. Thanks, Uros.
On 10/10/14 11:39, Uros Bizjak wrote: >> >> No objection. In fact, after reading everything it's pretty obvious to me >> that a /u MEM must be considered as potentially conflicting with writes that >> are implemented as RMW sequences to deal with the lack of byte access >> support. > > Thanks, I went ahead and commit the patch to SVN mainline. I wonder, > if they should be also committed to release branches? Do we have anything other than the ancient alphas that don't have byte access and thus have to generate the RMW sequences? If it's just the ancient alphas, I wouldn't bother. jeff
Index: alias.c =================================================================== --- alias.c (revision 216025) +++ alias.c (working copy) @@ -2458,18 +2458,6 @@ true_dependence_1 (const_rtx mem, enum machine_mod || MEM_ALIAS_SET (mem) == ALIAS_SET_MEMORY_BARRIER) return 1; - /* Read-only memory is by definition never modified, and therefore can't - conflict with anything. We don't expect to find read-only set on MEM, - but stupid user tricks can produce them, so don't die. */ - if (MEM_READONLY_P (x)) - return 0; - - /* If we have MEMs referring to different address spaces (which can - potentially overlap), we cannot easily tell from the addresses - whether the references overlap. */ - if (MEM_ADDR_SPACE (mem) != MEM_ADDR_SPACE (x)) - return 1; - if (! mem_addr) { mem_addr = XEXP (mem, 0); @@ -2493,6 +2481,22 @@ true_dependence_1 (const_rtx mem, enum machine_mod } } + /* Read-only memory is by definition never modified, and therefore can't + conflict with anything. However, don't assume anything when AND + addresses are involved and leave to the code below to determine + dependence. We don't expect to find read-only set on MEM, but + stupid user tricks can produce them, so don't die. */ + if (MEM_READONLY_P (x) + && GET_CODE (x_addr) != AND + && GET_CODE (mem_addr) != AND) + return 0; + + /* If we have MEMs referring to different address spaces (which can + potentially overlap), we cannot easily tell from the addresses + whether the references overlap. */ + if (MEM_ADDR_SPACE (mem) != MEM_ADDR_SPACE (x)) + return 1; + base = find_base_term (x_addr); if (base && (GET_CODE (base) == LABEL_REF || (GET_CODE (base) == SYMBOL_REF @@ -2576,16 +2580,6 @@ write_dependence_p (const_rtx mem, || MEM_ALIAS_SET (mem) == ALIAS_SET_MEMORY_BARRIER) return 1; - /* A read from read-only memory can't conflict with read-write memory. */ - if (!writep && MEM_READONLY_P (mem)) - return 0; - - /* If we have MEMs referring to different address spaces (which can - potentially overlap), we cannot easily tell from the addresses - whether the references overlap. */ - if (MEM_ADDR_SPACE (mem) != MEM_ADDR_SPACE (x)) - return 1; - mem_addr = XEXP (mem, 0); if (!x_addr) { @@ -2603,6 +2597,21 @@ write_dependence_p (const_rtx mem, } } + /* A read from read-only memory can't conflict with read-write memory. + Don't assume anything when AND addresses are involved and leave to + the code below to determine dependence. */ + if (!writep + && MEM_READONLY_P (mem) + && GET_CODE (x_addr) != AND + && GET_CODE (mem_addr) != AND) + return 0; + + /* If we have MEMs referring to different address spaces (which can + potentially overlap), we cannot easily tell from the addresses + whether the references overlap. */ + if (MEM_ADDR_SPACE (mem) != MEM_ADDR_SPACE (x)) + return 1; + base = find_base_term (mem_addr); if (! writep && base @@ -2690,18 +2699,6 @@ may_alias_p (const_rtx mem, const_rtx x) || MEM_ALIAS_SET (mem) == ALIAS_SET_MEMORY_BARRIER) return 1; - /* Read-only memory is by definition never modified, and therefore can't - conflict with anything. We don't expect to find read-only set on MEM, - but stupid user tricks can produce them, so don't die. */ - if (MEM_READONLY_P (x)) - return 0; - - /* If we have MEMs referring to different address spaces (which can - potentially overlap), we cannot easily tell from the addresses - whether the references overlap. */ - if (MEM_ADDR_SPACE (mem) != MEM_ADDR_SPACE (x)) - return 1; - x_addr = XEXP (x, 0); mem_addr = XEXP (mem, 0); if (!((GET_CODE (x_addr) == VALUE @@ -2715,6 +2712,22 @@ may_alias_p (const_rtx mem, const_rtx x) mem_addr = get_addr (mem_addr); } + /* Read-only memory is by definition never modified, and therefore can't + conflict with anything. However, don't assume anything when AND + addresses are involved and leave to the code below to determine + dependence. We don't expect to find read-only set on MEM, but + stupid user tricks can produce them, so don't die. */ + if (MEM_READONLY_P (x) + && GET_CODE (x_addr) != AND + && GET_CODE (mem_addr) != AND) + return 0; + + /* If we have MEMs referring to different address spaces (which can + potentially overlap), we cannot easily tell from the addresses + whether the references overlap. */ + if (MEM_ADDR_SPACE (mem) != MEM_ADDR_SPACE (x)) + return 1; + rtx x_base = find_base_term (x_addr); rtx mem_base = find_base_term (mem_addr); if (! base_alias_check (x_addr, x_base, mem_addr, mem_base,