[C11-atomic] [patch] gimple atomic statements

[Andrew MacLeod]

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