On 04/05/2012 05:14 AM, Richard Guenther wrote:
Ok. Remember that you should use non-tree things if you can in GIMPLE
land. This probably means that both the size and the memmodel "operands"
should be
+ struct GTY(()) gimple_statement_atomic
+ {
+ /* [ WORD 1-8 ] */
+ struct gimple_statement_with_memory_ops_base membase;
+
+ /* [ WORD 9 ] */
+ enum gimple_atomic_kind kind;
+
enum gimple_atomic_memmodel memmodel;
unsigned size;
and not be trees in the ops array. Even more, both kind and memmodel
should probably go in membase.base.subcode
I'm using subcode now for for the fetch_op and op_fetch operation when
we actually need a tree code for plus, sub, and, etc... so I am
utilizing that field. Since the 'kind' field is present in ALL node, and
the tree code was only needed in some, it looked cleaner to utilize the
subcode field for the 'sometimes' field so that it wouldnt be an obvious
wasted field in all the non-fetch nodes. In the end it amounts to the
same thing, but just looked cleaner :-) I could change if it you felt
strongly about it and use subcode for the kind and create a tree_code
field in the object for the operation.
Since integral atomics are always of an unsigned type , I could switch
over and use 'unsigned size' instead of 'tree fntype' for them (I will
rename it), but then things may be more complicated when dealing with
generic atomics... those can be structure or array types and I was
planning to allow leaving the type in case I discover something useful I
can do with it. It may ultimately turn out that the real type isn't
going to matter, in which case I will remove it and replace it with an
unsigned int for size.
And the reason memmodel is a tree is because, as ridiculous as it seems,
it can ultimately be a runtime value. Even barring that, it shows up
as a variable after inlining before various propagation engines run,
especially in the C++ templates. So it must be a tree.
I was actually thinking about doing it during gimplification... I hadnt
gotten as far as figuring out what to do with the functions from the front
end yet. I dont know that code well, but I was in fact hoping there was a
way to 'recognize' the function names easily and avoid built in functions
completely...
Heh ... you'd still need a GENERIC representation then. Possibly
a ATOMIC_OP tree may do.
possibly... or maybe a single generic atomic_builtin with a kind and a
variable list of parameters.
well, the names must remain exposed and recognizable since they are 'out
there'. Maybe under the covers I can just leave them as normal calls and
then during gimplification simply recognize the names and generate
GIMPLE_ATOMIC statements directly from the CALL_EXPR. That would be ideal.
That way there are no builtins any more.
I suppose my question was whether the frontends need to do sth about the
__atomic keyword or if that is simply translated to some type flag - or
is that keyword applying to operations, not to objects or types?
The _Atomic keyword is a type modifier like const or volatile. So
during gimplification I'll also look for all occurrences of variables in
normal expressions which have that bit set in the type, then translate
the expression to utilize the new gimple atomic node. so
_Atomic int var = 0;
var += 4;
foo (var);
would become
__atomic_add_fetch (&var, 4, SEQ_CST);
D.123 = __atomic_load (&var, SEQ_CST);
foo (D.123);
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 ;)
Not that I am aware of, they are 'noexcept'. But I'm sure I've missed more
than a few things so far. Im pretty early in the process :-)
What about compare-exchange on a pointer dereference? The pointer
dereference surely can trap, so it can throw with -fnon-call-exceptions. No?
in theory *any* gimple atomic operation could end up being expanded as a
library call, so I will have to ensure they are treated as calls because
of things like that.
These are all things I will focus on once all the basic functionality is
there. This patch is not meant to be fully flushed out, I just wanted
some eyes on it before I checked it into the branch so i dont carry this
huge patch set around when making changes. When I get things functional
in the branch I'll revisit all this and any of the other implementation
comments I don't get to and then submit another patch for review.
+ /* Return the expression field of atomic operation GS. */
+
+ static inline tree
+ gimple_atomic_expr (const_gimple gs)
+ {
+ GIMPLE_CHECK (gs, GIMPLE_ATOMIC);
+ gcc_assert (gimple_atomic_has_expr (gs));
+ return gimple_op (gs, 2);
+ }
err - what's "expression" in this context? I hope it's not an arbitrary
tcc_expression tree?!
Its just the 'expression' parameter of atomic operations which have it,
like store , fetch_add, or exchange. It would normally be an SSA_NAME
or const. I suppose we coud call it 'value' or something if expression
is confusing.
+ /* Return the arithmetic operation tree code for atomic operation GS. */
+
+ static inline enum tree_code
+ gimple_atomic_op_code (const_gimple gs)
+ {
+ GIMPLE_CHECK (gs, GIMPLE_ATOMIC);
+ gcc_assert (gimple_atomic_kind (gs) == GIMPLE_ATOMIC_FETCH_OP ||
+ gimple_atomic_kind (gs) == GIMPLE_ATOMIC_OP_FETCH);
+ return (enum tree_code) gs->gsbase.subcode;
+ }
now, what was it with this "expression" thing again? Btw, ||s go to the
next line. subcode should be the atomic kind - it seems that you glob
too much into GIMPLE_ATOMIC and that you maybe should sub-class
GIMPLE_ATOMIC properly via the subcode field. You'd have an
gimple_atomic_base which fences could use for example.
I was trying to make it simple and utilize the variable length ops array
to handle the variable stuff :-) It took many attempts to arrive at this
layout.
They don't really break down well into sub-components. I wanted to use
a single routine to access a given field for all atomic kinds, so
gimple_atomic_target(), gimple_atomic_lhs(), and gimple_atomic_expr()
just work . The problem is that they don't break down into a tree
structure, but more of a multiple-inheritance type thing.
LOAD has a LHS and a TARGET, no EXPR
STORE has no LHS, but has a TARGET and EXPR
EXCHANGE has a LHS, a TARGET and an EXPR
FENCE has no target or anything else.
COMPARE_EXCHANGE has 2 LHS, a TARGET, and an EXPR, not to mention an
additional memorder
I set it up so that the variable length array always stores a given
parameter at the same offset, and also allows for contiguous trees in
the array to represent all the LHS or RHS parameters for easy generic
operand scanning or whatever. And each statement type only allocated
the right amount of memory required.
I planned to add a comment to the description showing the layout of the
various nodes:
/*
LOAD | ORDER | TARGET | LHS |
STORE | ORDER | TARGET | EXPR |
EXCHANGE | ORDER | TARGET | EXPR | LHS |
COMPARE_EXCHG| ORDER | TARGET | EXPR | EXPECTED | FAIL_ORDER | LHS2
| LHS1 |
FETCH | ORDER | TARGET | EXPR | LHS |
TEST_AND_SET | ORDER | TARGET | LHS |
CLEAR | ORDER | TARGET |
FENCE | ORDER |
This allows all the RHS values to be contiguous for bulk processing like
operands, and located at the same index for an easy access routine. The
LHS is viewed from right to left, and allows more than 1 value as
required by compare_exchange. They also are contiguous for generic
access, and a single LHS routine can also be used to access those values.
*/
Is that OK? it seemed better than trying to map it to some sort of
hierarchical structure it doesn't really fit into.
Andrew
