Re: [PATCH] Reenable CSE of non-volatile inline asm (PR rtl-optimization/63637)

[Jeff Law <law@redhat.com>]

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