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
