On 01/13/15 17:03, Segher Boessenkool wrote:
On Tue, Jan 13, 2015 at 03:17:08PM -0700, Jeff Law wrote:
And finally there is the case of non-volatile asm with "memory" clobber
with
no memory stores in between the two - the posted (safer) patch will not
allow to CSE the two, while in theory we could CSE them into just one asm.
I think we have to assume that CSEing them is wrong.  The first may set
something in memory that is read by the second.

Thoughts?

I agree with pretty much everything you say in the thread, except for this
idea that a memory clobber reads memory.  No clobber reads anything.

The commit that introduced the memory clobber concept, 426b38c9 (svn 1207),
by rms, has as only comment

        /* `memory', don't cache memory across asm */
RMS botched this and you can see it in that the scheduler was not updated at the same time. The scheduler absolutely must track if an ASM does a memory read of an arbitrary location. I'd have to dig deeper to see when this got fixed, but it was clearly botched.


Many years later another pass which needs to precisely track such things came along, namely DSE.

The code in DSE is actually easier to grok.


First, if you look at the ASM handling in cfgexpand.c you'll find:

            if (j == -4)      /* `memory', don't cache memory across asm */
                {
                  XVECEXP (body, 0, i++)
                    = gen_rtx_CLOBBER (VOIDmode,
                                       gen_rtx_MEM
                                       (BLKmode,
                                        gen_rtx_SCRATCH (VOIDmode)));
                  continue;
                }


So we generate (CLOBBER (MEM:BLK (SCRATCH))) when we see "memory" in the "clobber" list of an ASM.


If you then look at dse.c we have this in record_store:

 /* At this point we know mem is a mem. */
  if (GET_MODE (mem) == BLKmode)
    {
      if (GET_CODE (XEXP (mem, 0)) == SCRATCH)
        {
          if (dump_file && (dump_flags & TDF_DETAILS))
fprintf (dump_file, " adding wild read for (clobber (mem:BLK (scratch))\n");
          add_wild_read (bb_info);
          insn_info->cannot_delete = true;
          return 0;
        }


Which says very precisely that we treat (CLOBBER (MEM:BLK (SCRATCH))) as potentially *reading* any location.

If you trace through how the scheduler builds dependencies, paying particular attention to alias.c you'll see that (CLOBBER (MEM:BLK (SCRATCH))) is treated as both a read and a write of an arbitrary location.

It's unfortunate that RMS put the "memory" tag in the "clobber" list. But he really wasn't a compiler junkie and didn't realize the right thing to do was to have a memory tag in both the inputs and [output|clobber] section to represent a read of an arbitrary location and a write to an arbitrary location independently. But it is what it is at this point and we have to treat "memory" appearing in the "clobber" list as an arbitrary memory read and an arbitrary memory write.



Jeff


Reply via email to