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

[Jeff Law <law@redhat.com>]

On 01/13/15 13:13, Jakub Jelinek wrote:
> On Tue, Jan 13, 2015 at 12:45:27PM -0700, Jeff Law wrote:
>> On 01/13/15 09:38, Segher Boessenkool wrote:
>>> On Tue, Jan 13, 2015 at 05:18:19PM +0100, Jakub Jelinek wrote:
>>>> 3) on request from Richard (which Segher on IRC argues against), "memory"
>>>>     clobber also prevents CSE;
>>>
>>> As extend.texi used to say:
>>>
>>> "
>>> If your assembler instructions access memory in an unpredictable
>>> fashion, add @samp{memory} to the list of clobbered registers.  This
>>> causes GCC to not keep memory values cached in registers across the
>>> assembler instruction and not optimize stores or loads to that memory.
>>> You also should add the @code{volatile} keyword if the memory
>>> affected is not listed in the inputs or outputs of the @code{asm}, as
>>> the @samp{memory} clobber does not count as a side-effect of the
>>> @code{asm}.
>>> "
>>>
>>> so a "memory" clobber in a non-volatile asm should not prevent CSE.
>> My reading of that paragraph is somewhat different.
>>
>> The key here is the memory clobber affects optimization of instructions
>> around the asm while the volatile specifier affects the optimization of the
>> ASM itself.
>>
>> A memory clobber must inhibit CSE of memory references on either side of the
>> asm because the asm must be assumed to read or write memory in unpredictable
>> ways.
>>
>> The volatile specifier tells the compiler that the asm itself must be
>> preserved, even if dataflow shows the outputs as not used.
>
> That is not necessarily in conflict.
Possibly not :-)  This stuff isn't trivial and as well meaning as folks 
trying to update the docs have been, it's possible subtle issues like 
these have been missed, even in the review process.

I know that for me it's easier to reason about code changes like this 
than their associated documentation :-)


> My reading of Jeff's comment is that in
> int a;
> int
> foo (void)
> {
>    int b, c, d, e;
>    b = a;
>    asm ("..." : "=r" (c) : : "memory");
>    d = a;
>    asm ("..." : "=r" (e) : : "memory");
>    return b + d + 2 * (c + e);
> }
> we are not allowed to CSE d = a; into d = b;
Precisely.  At least that's how I read things and it makes sense to the 
part of my brain that used to split time between kernel & GCC 
development in a previous life.

In effect the "memory" clobber is an aggregation of the read, write, 
clobbers dataflow for memory (and imprecise as it hits all memory).


.  CSE invalidate_from_clobbers
> should ensure that already, even when we don't do anything special about
> "memory" clobber in the patch.
OK.


   Another thing is if there is a store
> in between the two non-volatile asms with "memory" clobber, here I'm not
> sure if with the alternate patch we'd treat the "memory" clobber as use of
> everything previously stored into memory (in this regard the posted version
> is safe).
I woudln't be terribly surprised if DSE isn't safe in this regard.  I 
don't recall CSE doing any kind of dead store elimination so it wouldn't 
likely care that the memory clobber implies a read as well.



> 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?


Jeff