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

[Richard Guenther <richard dot guenther at gmail dot com>]

On Thu, Apr 26, 2012 at 7:53 PM, Andrew MacLeod <amacleod@redhat.com> wrote:
> 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.

So it eventually will support variable-size types?

> 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.

Ick.  That sounds gross.  So, if it ends up a variable at the time we generate
assembly we use a "conservative" constant value for it?

>
>>> 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);

Hmm, ok.  So you are changing GENERIC in fact.  I suppose _Atomic is
restricted to global variables.  Can I attach _Atomic to allocated storage
via pointer casts?

Consider writing up semantics of _Atomic into generic.texi please.

>
>
>
>
>>>
>>>>> 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.

Well, they are calls with a very well-defined set of side-effects.  Otherwise
not representing them as calls would be a waste of time.  Thus, no - they
do not need to be considered "calls" everywhere (well, treating them the
same as calls should be conservative) but treating them as "atomics" even
if they were expanded as calls needs to be correct.

> 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.

Ok, I see.

>>
>> + /* 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.

Or even 'op'.

>
>
>>
>> + /* 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

Looks like a hieararchy "no target or anything else" -> "target" ->
"expr" -> "memorder" would work, with only "lhs" being optional and
present everywhere.
Of course the hierarchy only makes sense for things that are not trees
(I was thinking of memorder originally - but that thought fell apart).
 So in the
end apart from abstracting a base for FENCE the flat hieararchy makes sense
(all but FENCE have a type / size).

> 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.

I suppose it's ok.  Please consider making a base available for FENCE
(does FENCE have a memorder?)

Richard.

> Andrew
>
>
>