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

[Richard Guenther <richard dot guenther at gmail dot com>]

On Wed, Apr 4, 2012 at 3:28 PM, Andrew MacLeod <amacleod@redhat.com> 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?

A memory reference, yes.  And at most one, thus no aggregate copies
anymore.

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

I'm not sure if I ever will end up finishing the above I just wanted
to mention it.

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

Oh, so you rather need a size or a mode specified, not a "fntype"?

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

You could use a vector, complex or aggregate return.

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

In the example you only ever use address operands (not memory operands)
to the GIMPLE_ATOMIC - is that true in all cases?  Is the result always
non-memory?

I suppose the GIMPLE_ATOMICs are still optimization barriers for all
memory, not just that possibly referenced by them?

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

I suppose gimple stmt folding could transform it as well?

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

As I understand you first generate builtins anyway and then lower them?
Or are you planning on emitting those for GENERIC as well?  Remember
GENERIC is not GIMPLE, so you'd need new tree codes anyway ;)
Or do you plan to make __atomic integral part of GENERIC and thus
do this lowering during gimplification?

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

Trivial?  Surely only as trivial as providing functions that do the task for
atomics - something which you can do for re-writing atomic builtins as well ...

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

But the builtins will stay for our users consumption and libstdc++ use, no?

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

You have internal functions now that are "untyped", I suppose atomics would
map to those easier given they are never emitted as calls but always explicitely
expanded.

> So bottom line, a GIMPLE_ATOMIC statement is just an object that is much
> easier to work with.

Yes, I see that it is easier to work with for you.  All other statements will
see GIMPLE_ATOMICs as blockers for their work though, even if they
already deal with calls just fine - that's why I most of the time suggest
to use builtins (or internal fns) to do things (I bet you forgot to update
enough predicates ...).  Can GIMPLE_ATOMICs throw with -fnon-call-exceptions?
I suppose yes.  One thing you missed at least ;)

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

Cleaner if you look at it in isolation - messy if you consider that not only
things working with atomics need to (not) deal with these new stmt kind.

Richard.

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