This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]