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

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

On Thu, Apr 26, 2012 at 10:07 PM, Andrew MacLeod <amacleod@redhat.com> wrote:
> On 04/05/2012 05:14 AM, Richard Guenther wrote:
>>
>> + static inline bool
>> + gimple_atomic_has_fail_order (const_gimple gs)
>> + {
>> + ? return gimple_atomic_kind (gs) == GIMPLE_ATOMIC_COMPARE_EXCHANGE;
>> + }
>>
>> btw, these kind of predicates look superfluous to me - if they are true
>> exactly for one atomic kind then users should use the predicate to
>> test for that specific atomic kind, not for some random field presence.
>
>
> yeah, a previous incarnation artifact, removed.
>
>>
>> All asserts in inline functions should be gcc_checking_asserts.
>
> indeed.
>
>>
>>
>> you should have a gimple_build_atomic_raw function that takes all
>> operands and the atomic kind, avoiding the need for all the repeated
>> calls of gimple_atomic_set_* as well as avoid all the repeated checking
>> this causes.
>
> as well. done.
>
>
>>
>> + ? else if (is_gimple_atomic (stmt))
>> + ? ? {
>> + ? ? ? tree t;
>> + ? ? ? if (visit_store)
>> + ? ? ? ? {
>> + ? ? ? ? for (i = 0; i< ?gimple_atomic_num_lhs (stmt); i++)
>> + ? ? ? ? ? {
>> + ? ? ? ? ? ? t = gimple_atomic_lhs (stmt, i);
>> + ? ? ? ? ? ? if (t)
>> + ? ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? ? t = get_base_loadstore (t);
>> + ? ? ? ? ? ? ? ? if (t)
>> + ? ? ? ? ? ? ? ? ? ret |= visit_store (stmt, t, data);
>> + ? ? ? ? ? ? ? }
>>
>> I thought results are always registers? ?The walk_stmt_load_store_addr_ops
>> looks wrong to me. ?As the loads/stores are implicit (you only have
>> addresses)
>> you have to adjust all callers of walk_stmt_load_store_addr_ops to handle
>> atomics specially as they expect to come along all loads/stores that way.
>> Those also handle calls and asms (for "memory" clobber) specially.
>
>
> hmm, yeah they always return a value. ? ? I was just copying the gimple_call
> code... ?Why would we need to do this processing ?for a GIMPLE_CALL lhs and
> not a GIMPLE_ATOMIC lhs?

GIMPLE_CALL lhs can be memory if the call returns an aggregate, similar
GIMPLE_CALL arguments can be memory if they are aggregate.

Can this be the case for atomics as well?

> ? And the RHS processing is the same as for a
> is_gimple_call as well... ?it was lifted from the code immediately
> following.,.. ?I tried to write all this code to return the same values as
> would have been returned had it actually been a ?built-in __sync or __atomic
> call like we have today. ?Once I had them all, then we could actually make
> any improvements based on our known side effect limits.
>
> I guess I don't understand the rest of the comment about why I need to do
> something different here than with a call...

Well, all callers that use walk_stmt_load_store/addr_ops need to handle
non-explicit loads/stores represented by the stmt.  For calls this includes
the loads and stores the callee may perform.  For atomics this includes .... ?
(depends on whether the operand of an atomic-load is a pointer or an object,
I suppose it is a pointer for the following) For atomic this includes the
implicit load of *{address operand} which will _not_ be returned by
walk_stmt_load_store/addr_ops.  Thus callers that expect to see all loads/stores
(like ipa-pure-const.c) need to explicitely handle atomics similar to how they
handle calls and asms (if only in a very conservative way).  Similar the
alias oracle needs to handle them (obviously).

>> Index: tree-ssa-alias.c
>> ===================================================================
>> *** tree-ssa-alias.c ? ?(revision 186098)
>> --- tree-ssa-alias.c ? ?(working copy)
>> *************** ref_maybe_used_by_stmt_p (gimple stmt, t
>> *** 1440,1445 ****
>> --- 1440,1447 ----
>> ? ? ? }
>> ? ? else if (is_gimple_call (stmt))
>> ? ? ? return ref_maybe_used_by_call_p (stmt, ref);
>> + ? else if (is_gimple_atomic (stmt))
>> + ? ? return true;
>>
>> please add a comment before these atomic handlings that we assume
>> they are using/clobbering all refs because they are considered memory
>> optimization barriers. ?Btw, does this apply to non-address-taken
>> automatic
>> references? ?I suppose not. ?Consider:
>>
>> int foo()
>> {
>> ? struct s;
>> ? atomic_fence();
>> ? s.a = 1;
>> ? atomic_fence();
>> ? return s.a;
>> }
>>
>> we still should be able to optimize this to return 1, no? ?At least SRA
>> will
>
> yes, locals can do anything they want since they aren't visible to other
> processes. ?at the moment, we'll leave those fences in because we dont
> optimize atomics at all, but ?"in the fullness of time" this will be
> optimized to:
> int foo()
> {
> ?atomic_fence()
> ?return 1;
> }
>
> at the moment we produce:
>
> int foo()
> {
> ?atomic_fence()
> ?atomic_fence()
> ?return 1;
>
> }

Which is inconsistend with the alias-oracle implementation.  Please fix it
to at _least_ not consider non-aliased locals.  You can look at the call
handling for how to do this - where you can also see how to do even better
by using points-to information from the pointer operand of the atomics
that specify the memory loaded/stored.  You don't want to hide all your
implementation bugs by making the alias oracle stupid ;)

>>
>> *************** stmt_kills_ref_p_1 (gimple stmt, ao_ref
>> *** 1814,1819 ****
>> --- 1818,1825 ----
>> ? ? ? ? }
>> ? ? ? }
>>
>> + ? if (is_gimple_atomic (stmt))
>> + ? ? return true;
>>
>> that's for sure wrong ;) ?It should return false.
>
>
> err, yeah. oops.
>
>>
>> Index: tree-ssa-sink.c
>> ===================================================================
>> *** tree-ssa-sink.c ? ? (revision 186098)
>> --- tree-ssa-sink.c ? ? (working copy)
>> *************** is_hidden_global_store (gimple stmt)
>> *** 145,150 ****
>> --- 145,154 ----
>> ? ? ? {
>> ? ? ? ? tree lhs;
>>
>> + ? ? ? /* Don't optimize across an atomic operation. ?*/
>> + ? ? ? if (is_gimple_atomic (stmt))
>> + ? ? ? ? return true;
>> +
>>
>> that's bogus, too (really all uses of is_hidden_global_store should go
>> away).
>> Please look into the few callers of this function and ?handle atomics in
>> a correct way explicitely.
>
> All atomic operations will have a VDEF so in theory it should be fine to
> ignore.

Ignore?  Returning 'false' would be ignoring them.  Why do all atomic
operations have a VDEF?  At least atomic loads are not considered
stores, are they?

> ?There are only 2 uses:
> ?DCE already has code added to handle them, and
> ?DSE handles it through ref_maybe_used_by_stmt_p.

Yes.  And eventually what matters for both callers should be folded into them
(let me put that somewhere ontop of my TODO ...)

For example DCE would happily remove atomic loads if they look like

 result-SSA-name = ATOMIC-LOAD <ptr-SSA-name>;

if result-SSA-name is not used anywhere.  And I don't see why we should
not be allowed to do this?  Returning true from the above function will
make DCE not remove it.

> removed.
>
>
>>
>> + ? ? ? ? else if (is_gimple_atomic (stmt))
>> + ? ? ? ? ? {
>> + ? ? ? ? ? ? unsigned n;
>> +
>> + ? ? ? ? ? ? /* We may be able to lessen this with more relaxed memory
>> + ? ? ? ? ? ? ? ?models, but for now, its a full barrier. ?*/
>> + ? ? ? ? ? ? mark_all_reaching_defs_necessary (stmt);
>> +
>> + ? ? ? ? ? ? for (n = 0; n< ?gimple_atomic_num_rhs (stmt); n++)
>> + ? ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? ? tree t = gimple_op (stmt, n);
>> + ? ? ? ? ? ? ? ? if (TREE_CODE (t) != SSA_NAME&&
>> + ? ? ? ? ? ? ? ? ? ? TREE_CODE (t) != INTEGER_CST&&
>> + ? ? ? ? ? ? ? ? ? ? !is_gimple_min_invariant (t)&&
>> + ? ? ? ? ? ? ? ? ? ? !ref_may_be_aliased (t))
>> + ? ? ? ? ? ? ? ? ? mark_aliased_reaching_defs_necessary (stmt, t);
>> + ? ? ? ? ? ? ? }
>> + ? ? ? ? ? }
>>
>> for sure atomics do not make non-aliased automatic variable stores
>> necessary.
>> At least I hope so. ?As there are only addresses in the atomic ops the
>> code looks wrong to me (and&&s go to the next line ...). ?As you are
>>
>> marking everything needed anway you can just remove the bogus loop
>> completely.
>
> I was simply copying the code path that was followed for gimple_call which
> handled the old __sync_ and __atomic builtins... ?this is what that code
> did. ?there are addresses in the atomic ops, but the functionality of those
> operands can dereference and store or load from those address...?
> ie ?the generic
> ?atomic_store (&atomic, &expr)
> dereferences ?*expr and stores it into *atomic...

Sure.  But it only can ever load/store from aliased variables (not
non-address-taken
local automatics).  That we did not handle this for the __sync_ atomics very
well is no excuse to not handle it well for atomics ;)

> I figured whatever I can see as a function call argument to __atomic_* would
> be seeable in a GIMPLE_ATOMIC position as well...?

Yes.  But you know - compared to a random call - there are no other side-effects
on memory.

>> + ? ? case GIMPLE_ATOMIC:
>> + ? ? ? /* Treat this like a call for now, it may expand into a call. ?*/
>> + ? ? ? if (gimple_atomic_kind (stmt) != GIMPLE_ATOMIC_FENCE)
>> + ? ? ? cost = gimple_num_ops (stmt) *
>> + ? ? ? ? ? ? ?estimate_move_cost (TREE_TYPE (gimple_atomic_target
>> (stmt)));
>> + ? ? ? else
>> + ? ? ? ? cost = 1;
>> + ? ? ? break;
>>
>> for sure this counts way too many "ops". ?There is at most a single
>> memory operand as far as I understand. ?A size/speed cost of 1 for
>> a FENCE is also too low.
>
> These were just fillers values to get it to compile until I had time to
> understand the proper way to find these values. ?I thought there were just
> instruction count guesses, and again mimiced what the gimple_call code did.

Sure, still it does not make sense to account for "move" cost of the memorder
operand the same as for the "move" cost of the eventual load/store that
happens.  If it's a filler just fill in 'cost = 10' with a comment.

>
>> I miss handling of GIMPLE_ATOMICs in tree-ssa-structalias.c.
>>
>>
> No doubt I havent gotten there yet because it did cause a compilation
> failure :-) :-)

;)  It will only cause ICEs and miscompiles.

Richard.

> Andrew