On 04/27/2012 04:52 AM, Richard Guenther wrote:
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?
Ahhhh. point taken. No. No aggregates can ever be returned by an
atomic, so yes, I can trim that out then.
When generic atomics are used to handle aggregates, it's all handled by
pointer parameters and the function is usually void, except for
compare_exchange which returns a boolean.
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 .... ?
the only implicit thing a normal atomic operation can do is load or
store the TARGET.
When the type is an aggregate, then the EXPR (soon to be op :-) and/or
EXPECTED field may also be loaded or stored indrectly as all the
parameters become pointers. When I add that code I will reflect that
properly.
(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).
OK, I understand better :-)
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 ;)
I'm too transparently lazy obviously :-) I'll look at it :-)
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?
very true, and neither do fences... But we can't move any shared memory
load or store past them, so making them all VDEFS prevents that
activity. Once gimple_atomic is working properly, it might be possible
to go back and adjust places. In particular, we may be able to allow
directional movement based on the memory order. seq_cst will always
block motion in both directions, but the acquire model allows shared
memory accesses to be sunk, and the release model allows shared memory
access to be hoisted. relaxed allows both.. We may be able to get this
behaviour through appropriate uses of VDEFs and VUSES... or maybe it
will be through a more basic understanding of GIMPLE_ATOMIC in the
appropriate places. I will visit that once everything is working
conservatively.
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.
at the moment, we are NOT allowed to remove any atomic operation. There
are synchronization side effects and the only way we can remove such a
statement is by analyzing the atomic operations in relation to each
other. (ie, 2 loads from the same atomic with no other intervening
atomic operation may make the first one dead if the memory orders are
compatible)
I will also revisit this once we have a set of operations and conditions
upon which we can operate. It may be possible to simply remove this
load, but for the moment it stays. My optimization wiki page indicates
that a dead atomic load might be removable.. I just haven't had
confirmation from 'experts' on atomic operations yet, so I won't act on
that yet.
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 ;)
absolutely. And Im going to be rolling all the __sync builtins as well
eventually as well. I was just explaining where the code came from, not
that its optimal :-) I'll remove the bogus loop :-)
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.
will do.
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.
err, I meant to say did NOT ICE. I'll have a look at what I need in
tree-ssa-structalias.c
Thanks
Andrew
