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? 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...
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;
}
*************** 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. There are only 2 uses:
DCE already has code added to handle them, and
DSE handles it through ref_maybe_used_by_stmt_p.
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...
I figured whatever I can see as a function call argument to __atomic_*
would be seeable in a GIMPLE_ATOMIC position as well...?
+ 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.
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 :-) :-)
Andrew
