This is the mail archive of the gcc-bugs@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[Bug target/63483] Scheduler performs Invalid move of aliased memory reference


https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63483

--- Comment #14 from rguenther at suse dot de <rguenther at suse dot de> ---
On Thu, 9 Oct 2014, ubizjak at gmail dot com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63483
> 
> --- Comment #13 from UroÅ Bizjak <ubizjak at gmail dot com> ---
> (In reply to Richard Biener from comment #12)
> > I still don't get it.  What we end up doing is
> > 
> >   reg:DI 72 = /u load from 'a'  (insn 6)
> >   reg:DI 78 = /u load from 'b'  (insn 15)
> > ...
> >   RMW sequence on (mem:DI (reg:DI 72 ...
> >   store to (mem:SI (reg:DI 78 ...
> > 
> > thus completely fine (after sched1).  sched2 then moves the /u load from
> > 'b' until after the R of the RMW sequence, but that's fine.
> 
> This is wrong. The move should not pass the W part of the RMW sequence.
> 
> > Now I think you seem to say that this isn't about /u or whatever but
> > the common issue that for example for globals
> > 
> > char c, d;
> > 
> > two RMW sequences to store to c and d may not overlap if 'c' and 'd' are
> > not padded out to DImode.
> > 
> > But this is an issue on all targets (ISTR a bug about this), and if
> > that happens it introduces a store data race that is not allowed with
> > the C++ memory model for example.  Bad luck for early alphas then I'd say.
> 
> Yes, this is the problem. For the original problem, "c" and "d" live in the
> same 8-byte memory location. So memory access with 8-byte granularity aliases
> both values. There is a code that prevents memory reads to be moved across
> memory writes with alignment ANDs, but /u reads get treated specially and
> bypass this code under "can't alias anything due to constness" assumption.
> 
> > At least I can see nothing wrong with the code generated for the two
> > testcases
> > you provided nor is the fix you propose any good (I see nothing else but
> > the (and:DI ...) addressing that might possibly serve as "barrier" here,
> > but even that would not be enough for, say
> > 
> > char c __attribute__((aligned(8)));
> > char d;
> > 
> > because then we hopefully optimize that AND away for the load from c.
> 
> True, but there will be alignment ANDs in the access of "d". This is the reason
> to check both operands for ANDs and in this case pass them down to the code
> that handles operands with alignment ANDs to determine their aliasing.
> 
> > Can you clarify whether I miss anything with the code generated for the
> > testcase?
> 
> If we can't guarantee that the value lives outside aliased location, then reads
> of this value should not pass writes with alignment ANDs (that could possibly
> alias the location of read), no matter if they are /u or not.
> 
> IMO, /u reads should be handled as non-/u reads and should not pass writes with
> alignment ANDs. This is what my patch does.

I see.  But you don't touch write_dependence which has a similar check.
And instead of making the check for AND disable the MEM_READONLY_P
early-out wouldn't it be more correct to treat MEMs with AND addresses
as true barriers?  That is,

 if (GET_CODE (x_addr) == AND
     || GET_CODE (mem_addr) == AND)
   return 1;

 if (MEM_READONLY_P (x))
   return 0;

instead?  I see we're trying to be clever with AND addresses even
though they are "barriers", why's that?

Note that all this should only ever be an issue with that very old Alpha
as other archs should implement the C++ memory model properly.

Richard.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]