Re: [C11-atomic] [patch] gimple atomic statements

[Andrew MacLeod <amacleod@redhat.com>]

Im not sure what happened to my original reply, so I'll resend it..


On 04/04/2012 09:28 AM, Andrew MacLeod wrote:
On 04/04/2012 04:45 AM, Richard Guenther wrote:
>
> The fact that you need to touch every place that wants to look at memory
> accesses shows that you are doing it wrong.  Instead my plan was to
> force _all_ memory accesses to GIMPLE_ASSIGNs (yes, including those
> we have now in calls).  You're making a backwards step in my eyes.
I'm not sure I understand what you are saying, or at least I don't know 
what this plan you are talking about is...   Are you saying that you are 
planning to change gimple so that none of the gimple statement types 
other than GIMPLE_ASSIGN ever see an ADDR_EXPR or memory reference?     
Seems like that change, when it happens, would simply affect 
GIMPLE_ATOMIC like all the other gimple classes.  And if it was done 
before I tried to merge the branch, would fall on me to fix.  Right now, 
I'm just handling what the compiler sends my way...  A bunch of places 
need to understand a new gimple_statement kind...
> What do you think is "easier" when you use a GIMPLE_ATOMIC
> (why do you need a fntype field?!  Should the type not be available
> via the operand types?!)

This is a WIP... that fntype fields is there for simplicity..   and 
no... you can do a 1 byte atomic operation on a full word object if you 
want by using __atomic_store_1 ()... so you can't just look at the 
object. We might be able to sort that type out eventually if all the 
casts are correct, but until everything is finished, this is safer.  I'm 
actually hoping eventually to not have a bunch of casts on the params, 
they are just there to get around the builtin's type-checking system.. 
we should be able to  just take care of required promotions at expansion 
time and do type-checking during verification.

>
> Your tree-cfg.c parts need to be filled in.  They are the 
> specification of
> GIMPLE_ATOMIC - at the moment you allow any garbage.

well of course.... this isnt suppose to be a final patch, its to get the 
core changes into a branch while I continue working on it.  There are a 
number of parts that aren't filled in or flushed out yet.   Once its all 
working and what is expected is well defined, then I'll fill in the 
verification stuff.

> Similar to how I dislike the choice of adding GIMPLE_TRANSACTION
> instead of using builtin functions I dislike this.
>
> I suppose you do not want to use builtins because for primitive types you
> end up with multiple statements for something "atomic"?
builtins are just more awkward to work with, and don't support more than 
1 result.
compare_and swap was the worst case.. it has 2 results and that does not 
map to a built in function very well. we struggled all last fall with 
how to do it efficiently, and eventually gave up. given:

   int p = 1;
   bool ret;
   ret = __atomic_compare_exchange_n (&flag2, &p, 0, 0, 
__ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);
   return ret;

with GCC 4.7 we currently end up generating

   p = 1;
   ret_1 = __atomic_compare_exchange_4 (&flag2, &p, 0, 0, 5, 5);
   return ret_1;

Note this actually requires leaving a local (p) on the stack, and 
reduces the optimizations that can be performed on it, even though there 
isn't really a need.

By going to a gimple statement, we can expose both results properly, and 
this ends up generating

   (ret_3, cmpxchg.2_4) = atomic_compare_exchange_strong <&flag2, 1, 0, 
SEQ_CST, SEQ_CST>
   return ret_3;

and during expansion to RTL, can trivially see that cmpxchg.2_4 is not 
used, and generate the really efficient compare and swap pattern which 
only produces a boolean result.   if only cmpxchg.2_4 were used, we can 
generate the C&S pattern which only returns the result.  Only if we see 
both are actually used do we have to fall back to the much uglier 
pattern we have that produces both results.  Currently we always 
generate this pattern.

Next, we have the C11 atomic type qualifier which needs to be 
implemented.  Every reference to this variable is going to have to be 
expanded into one or more atomic operations of some sort.  Yes, I 
probably could do that by emitting built-in functions, but they are a 
bit more unwieldy, its far simpler to just create gimple_statements.

I discovered last fall that when I tried to translate one built-in 
function into another form that dealing with parameters and all the call 
bits was a pain.  Especially when the library call I need to emit had a 
different number of parameters than the built-in did.   A GIMPLE_ATOMIC 
statement makes all of this trivial.

I also hope that when done, I can also remove all the ugly built-in 
overload code that was created for __sync and continues to be used by 
__atomic.  This would clean up where we have to take func_n and turn it 
into a func_1 or func_2 or whatever.  We also had to bend over and issue 
a crap load of different casts early to squeeze all the parameters into 
the 'proper' form for the builtins. This made it more awkward to dig 
down and find the things being operated on and manipulate them. The 
type-checking code is not a thing of beauty either.   Expansion of 
GIMPLE_ATOMIC should take care of cleaning all that up.

So bottom line, a GIMPLE_ATOMIC statement is just an object that is much 
easier to work with.  It cleans up both initial creation and rtl 
generation, as well as being easier to manipulate. It also encompasses 
an entire class of operations that are becoming more integral *if* we 
can make them efficient, and I hope to actually do some optimizations on 
them eventually.  I had a discussion last fall with Linus about what we 
needed to be able to do to them in order for the kernel to use __atomic 
instead of their home-rolled solutions.  Could I do everything with 
builtins? sure... its just more awkward and this approach seems cleaner 
to me.

I wasn't excited about creating a new gimple statement, but it seemed 
the best solution to my issues. In the end, I think this works very 
cleanly.  Im certainly open to better solutions. If there is a plan to 
change gimple in some way that this doesnt work with, then it would be 
good to know what that plan is.

Andrew



>