New post-LTO OpenACC pass

On 09/21/15 16:39, Nathan Sidwell wrote:
> On 09/21/15 16:30, Cesar Philippidis wrote:
>> On 09/21/2015 09:30 AM, Nathan Sidwell wrote:
>>
>>> +const pass_data pass_data_oacc_transform =
>>> +{
>>> +  GIMPLE_PASS, /* type */
>>> +  "fold_oacc_transform", /* name */
>>
>> Want to rename the tree dump file to oacc_xforms like I'm did in the
>> attached patch? Regardless, I think we need to document this flag in
>> invoke.texi.
>
> Thanks for noticing the missing doc.  I'm not attached to any particular name.
> 'fold_oacc_transform' is rather generic, and a bit of  a mouthful.  Perhaps
> 'oacclower', 'oaccdevlower' or something (I  see there's 'lateomplower' for
> guidance)

this updated patch includes Cesar's doc patch.  Also change the name of the pass 
to 'oaccdevlow'.

nathan

On 09/22/2015 05:16 PM, Nathan Sidwell wrote:
> +	if (gimple_call_builtin_p (call, BUILT_IN_ACC_ON_DEVICE))
> +	  /* acc_on_device must be evaluated at compile time for
> +	     constant arguments.  */
> +	  {
> +	    oacc_xform_on_device (call);
> +	    rescan = true;
> +	  }

Is there a reason this is not done as part of pass_fold_builtins? (It 
looks like maybe adding this to fold_call_stmt in builtins.c would be 
sufficient too).


Bernd

On 09/23/15 06:59, Bernd Schmidt wrote:
> On 09/22/2015 05:16 PM, Nathan Sidwell wrote:
>> +    if (gimple_call_builtin_p (call, BUILT_IN_ACC_ON_DEVICE))
>> +      /* acc_on_device must be evaluated at compile time for
>> +         constant arguments.  */
>> +      {
>> +        oacc_xform_on_device (call);
>> +        rescan = true;
>> +      }
>
> Is there a reason this is not done as part of pass_fold_builtins? (It looks like
> maybe adding this to fold_call_stmt in builtins.c would be sufficient too).

Perhaps it could be.  I'll need to check where  that pass happens.  Anyway, the 
main thrust of this patch is the new pass, which I thought might be easier to 
review with minimal additional  clutter.

nathan

On 09/23/2015 02:14 PM, Nathan Sidwell wrote:
> On 09/23/15 06:59, Bernd Schmidt wrote:
>> On 09/22/2015 05:16 PM, Nathan Sidwell wrote:
>>> +    if (gimple_call_builtin_p (call, BUILT_IN_ACC_ON_DEVICE))
>>> +      /* acc_on_device must be evaluated at compile time for
>>> +         constant arguments.  */
>>> +      {
>>> +        oacc_xform_on_device (call);
>>> +        rescan = true;
>>> +      }
>>
>> Is there a reason this is not done as part of pass_fold_builtins? (It
>> looks like
>> maybe adding this to fold_call_stmt in builtins.c would be sufficient
>> too).
>
> Perhaps it could be.  I'll need to check where  that pass happens.
> Anyway, the main thrust of this patch is the new pass, which I thought
> might be easier to review with minimal additional  clutter.

There's no issue adding a new pass if there's a demonstrated need for 
it, but I think builtin folding doesn't quite meet that criterion given 
that we already have a pass that does that. Unless you really need it to 
happen very early in the pipeline - fold_builtins runs pretty late, but 
I checked and fold_call_stmt gets called from pass_forwprop and possibly 
from elsewhere too.


Bernd

On 09/23/15 08:58, Bernd Schmidt wrote:
> On 09/23/2015 02:14 PM, Nathan Sidwell wrote:
>> On 09/23/15 06:59, Bernd Schmidt wrote:
>>> On 09/22/2015 05:16 PM, Nathan Sidwell wrote:
>>>> +    if (gimple_call_builtin_p (call, BUILT_IN_ACC_ON_DEVICE))
>>>> +      /* acc_on_device must be evaluated at compile time for
>>>> +         constant arguments.  */
>>>> +      {
>>>> +        oacc_xform_on_device (call);
>>>> +        rescan = true;
>>>> +      }
>>>
>>> Is there a reason this is not done as part of pass_fold_builtins? (It
>>> looks like
>>> maybe adding this to fold_call_stmt in builtins.c would be sufficient
>>> too).


As I feared, builtin folding occurs in several places.  In particular its first 
call is very early on in the host compiler, which is far too soon.

We have to defer folding until we know whether we're doing host or device 
compilation.

nathan

On 09/23/2015 08:42 PM, Nathan Sidwell wrote:

> As I feared, builtin folding occurs in several places.  In particular
> its first call is very early on in the host compiler, which is far too
> soon.
>
> We have to defer folding until we know whether we're doing host or
> device compilation.

Doesn't something like "symtab->state >= EXPANSION" give you that?


Bernd

On 09/23/15 14:51, Bernd Schmidt wrote:
> On 09/23/2015 08:42 PM, Nathan Sidwell wrote:
>
>> As I feared, builtin folding occurs in several places.  In particular
>> its first call is very early on in the host compiler, which is far too
>> soon.
>>
>> We have to defer folding until we know whether we're doing host or
>> device compilation.
>
> Doesn't something like "symtab->state >= EXPANSION" give you that?

I don't know.   It doesn't seem to me to be a good idea for the builtin 
expanders to be context-sensitive.

nathan

On 09/23/15 14:58, Nathan Sidwell wrote:
> On 09/23/15 14:51, Bernd Schmidt wrote:
>> On 09/23/2015 08:42 PM, Nathan Sidwell wrote:
>>
>>> As I feared, builtin folding occurs in several places.  In particular
>>> its first call is very early on in the host compiler, which is far too
>>> soon.
>>>
>>> We have to defer folding until we know whether we're doing host or
>>> device compilation.
>>
>> Doesn't something like "symtab->state >= EXPANSION" give you that?

I've tried limiting expansion by checking symtab->state.  I have been unable to 
succeed.

It either expands too early in the host compiler, or it doesn't get expanded at 
  all and one ends up with an RTL call to the library function.   For instance 
there doesn't appear to be call to fold builtins when state == EXPANSION. 
lesser values are present in the host compiler before LTO write out, AFAICT.

nathan

On 09/25/2015 12:38 AM, Nathan Sidwell wrote:
> On 09/23/15 14:58, Nathan Sidwell wrote:
>> On 09/23/15 14:51, Bernd Schmidt wrote:
>>> On 09/23/2015 08:42 PM, Nathan Sidwell wrote:
>>>> We have to defer folding until we know whether we're doing host or
>>>> device compilation.
>>>
>>> Doesn't something like "symtab->state >= EXPANSION" give you that?
>
> I've tried limiting expansion by checking symtab->state.  I have been
> unable to succeed.
>
> It either expands too early in the host compiler, or it doesn't get
> expanded at  all and one ends up with an RTL call to the library
> function.   For instance there doesn't appear to be call to fold
> builtins when state == EXPANSION. lesser values are present in the host
> compiler before LTO write out, AFAICT.

That's a bit odd:

Breakpoint 5, (anonymous namespace)::pass_fold_builtins::execute 
(this=0x1ce89a0, fun=0x7ffff0858348) at ../../git/gcc/tree-ssa-ccp.c:2722
[...]
(gdb) p stmt
$3 = (gimple *) 0x7ffff0736d80
(gdb) pgg
warning: Expression is not an assignment (and might have no effect)
# .MEM_2 = VDEF <.MEM_1(D)>
_3 = acc_on_device (123);
(gdb) p symtab->state
$4 = EXPANSION

On the other hand, it's not considered a builtin:

(gdb) p gimple_call_builtin_p(stmt, BUILT_IN_ACC_ON_DEVICE)
$6 = false

This is the c-c++-common/goacc/acc_on_device-2.c testcase. Is that 
expected to be handled? If I change it to use __builtin_acc_on_device, I 
can step right into

Breakpoint 8, fold_call_stmt (stmt=0x7ffff0736e10, ignore=false) at 
../../git/gcc/builtins.c:12277
12277	  tree ret = NULL_TREE;

Maybe you were compiling without optimization? In that case 
expand_builtin_acc_on_device (which already exists) should still end up 
doing the right thing. In no case should you see a RTL call to a 
function, that indicates that something else went wrong.

Can you send me the patch you tried (and possibly a testcase you expect 
to be handled), I'll see if I can find out what's going on.


Bernd