On Thu, Feb 2, 2012 at 8:00 PM, Patrick Marlier
<patrick.marl...@gmail.com> wrote:
> On 02/02/2012 04:22 AM, Richard Guenther wrote:
>>
>> On Wed, Feb 1, 2012 at 10:19 PM, Patrick Marlier
>> <patrick.marl...@gmail.com> wrote:
>>>
>>> On 02/01/2012 03:59 AM, Richard Guenther wrote:
>>>>
>>>>
>>>> The patch looks ok, but I'm not sure why you do not get a cgraph node
>>>> here - cgraph doesn't really care for builtins as far as I can see.
>>>> Honza?
>>>
>>>
>>> I cannot help on this...
>>>
>>>
>>>> Don't you maybe want to handle builtins in a special way here?
>>>
>>>
>>> Indeed, I think my patch is wrong. __builtin_prefetch should have the
>>> transaction_pure attribute. I don't know how usually it should be done
>>> but
>>> what about adding a gcc_assert before to dereference node (potentially
>>> NULL)?
>>>
>>> How the attached patch looks like now?
>>> (Tested on i686)
>>>
>>>
>>>> At least those that are const/pure?
>>>
>>> About const/pure, we cannot consider those functions as transaction_pure
>>> because they can read global and shared variable.
>>
>>
>> Well, const functions cannot access global memory, they can only inspect
>> their arguments.
>
>
> Actually, in this example, GCC does not complain at all about those const
> functions which read global memory... Should it? or the user is allowed to
> indicate const even if it is not?
Yes, it's the users fault if he does something wrong (after all GCC may
not even see the definition). GCC will simply assume it doesn't touch
global memory and thus optimize
a = 1;
b = f (&a);
a = 2;
if (b != f(&a))
to if (0).
> static int a = 0;
>
> int f (int* a) __attribute__((const));
> int f (int* a)
> {
> return *a+1;
> }
>
> int g () __attribute__((const));
> int g ()
> {
> return a+1;
> }
>
> int main()
> {
> int tmp = 0;
> __transaction_atomic {
> tmp += f(&a);
> tmp += g();
> }
>
> return tmp;
> }
>
> In this test, the transaction is completely removed since no transactional
> operation is detected.
>
>
>> Of course __builtin_prefetch seems to be special in some way. Note that
>> users can explicitely call it, so setting the attribute from the
>> prefetching
>> pass isn't the correct thing to do.
>
> If the user calls it explicitly, at least the compiler doesn't ICE.
> Ex: error: unsafe function call ‘__builtin_prefetch’ within atomic
> transaction.
> So what's your proposal? to add directly in builtins.def the
> transaction_pure attribute to required functions?
No, have a transaction_pure_function_p predicate which checks for
the attribute and also has an explicit list of builtin functions that
are considered transaction pure.
>> Note that __builtin_prefetch has the 'no vops' attribute - I think you
>> should
>> simply consider all 'no vops' builtins as transaction pure instead or
>> explicitely
>> consider a set of builtins as transaction pure (that's more scalable than
>> sticking the attribute onto random builtins - see how we handle builtins
>> in
>> the alias machinery, we avoid sticking the fnspec attribute onto each
>> builtin but simply have special handling for them).
>
>
> 'no vops' attribute description is quite vague. "may have arbitrary side
> effects" scares me ;)
> I will have a look at this alias machinery (any files particularly? tree.c
> seems to have a lot of builtins things in it).
We don't seem to have many uses of "no vops" left, so I suppose we
should eventually remove it in favor of something more precise ;)
> In this patch, I do manage 'no vops' the same way as const is.
> Attached the new version where we add ECF_TM_PURE to NOVOPS functions.
> Tested on x86_64-unknown-linux-gnu.
Looks good to me.
Thanks,
Richard.
> Thanks!
> Patrick.
>
> 2012-02-02 Patrick Marlier <patrick.marl...@gmail.com>
>
>
> PR middle-end/52047
> * trans-mem.c (expand_call_tm): Add an assertion.
> * calls.c (flags_from_decl_or_type): Add ECF_TM_PURE to 'no vops'
> functions.
>
>
>>
>> Thanks,
>> Richard.
>>
>>> BTW, I will post a PR (and probably a patch) about this.
>>>
>>> Thanks for your comment!
>>>
>>> Patrick.
>>>
>>> PR middle-end/52047
>>> * trans-mem.c (expand_call_tm): Add an assertion.
>>> * tree-ssa-loop-prefetch.c (issue_prefetch_ref): Add
>>> transaction_pure
>>> attribute to __builtin_prefetch.
>>> (tree_ssa_prefetch_arrays): Likewise.
>
>
