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.


Reply via email to