Fold acc_on_device

Fold acc_on_device

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 345 bytes --]

This patch folds acc_on_device as a regular builtin, but postponed until we know 
which compiler we're in.  As suggested by Bernd, we use the existing builtin 
folding machinery.

Trunk is still using  the older PTX runtime scheme (Thomas is working on that), 
so the only change there is in the  host-side libgomp piece.

Ok for trunk?

nathan

[-- Attachment #2: trunk-ondev.patch --]
[-- Type: text/x-patch, Size: 3781 bytes --]

2015-09-29  Nathan Sidwell  <nathan@codesourcery.com>

	gcc/
	* builtins.c (expand_builtin_acc_on_device): Delete.
	(expand_builtin): Don't call it.
	(fold_builtin_1): Fold acc_on_device.

	libgomp/
	* oacc-init.c (acc_on_device): Force optimization level.

Index: libgomp/oacc-init.c
===================================================================
--- libgomp/oacc-init.c	(revision 228250)
+++ libgomp/oacc-init.c	(working copy)
@@ -620,10 +620,12 @@ acc_set_device_num (int ord, acc_device_
 
 ialias (acc_set_device_num)
 
-int
+/* Compile on_device with optimization, so that the compiler expands
+   this, rather than generating infinitely recursive code.  */
+
+int __attribute__ ((__optimize__ ("O2")))
 acc_on_device (acc_device_t dev)
 {
-  /* Just rely on the compiler builtin.  */
   return __builtin_acc_on_device (dev);
 }
 
Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	(revision 228250)
+++ gcc/builtins.c	(working copy)
@@ -5859,46 +5859,6 @@ expand_stack_save (void)
 }
 
 
-/* Expand OpenACC acc_on_device.
-
-   This has to happen late (that is, not in early folding; expand_builtin_*,
-   rather than fold_builtin_*), as we have to act differently for host and
-   acceleration device (ACCEL_COMPILER conditional).  */
-
-static rtx
-expand_builtin_acc_on_device (tree exp, rtx target)
-{
-  if (!validate_arglist (exp, INTEGER_TYPE, VOID_TYPE))
-    return NULL_RTX;
-
-  tree arg = CALL_EXPR_ARG (exp, 0);
-
-  /* Return (arg == v1 || arg == v2) ? 1 : 0.  */
-  machine_mode v_mode = TYPE_MODE (TREE_TYPE (arg));
-  rtx v = expand_normal (arg), v1, v2;
-#ifdef ACCEL_COMPILER
-  v1 = GEN_INT (GOMP_DEVICE_NOT_HOST);
-  v2 = GEN_INT (ACCEL_COMPILER_acc_device);
-#else
-  v1 = GEN_INT (GOMP_DEVICE_NONE);
-  v2 = GEN_INT (GOMP_DEVICE_HOST);
-#endif
-  machine_mode target_mode = TYPE_MODE (integer_type_node);
-  if (!target || !register_operand (target, target_mode))
-    target = gen_reg_rtx (target_mode);
-  emit_move_insn (target, const1_rtx);
-  rtx_code_label *done_label = gen_label_rtx ();
-  do_compare_rtx_and_jump (v, v1, EQ, false, v_mode, NULL_RTX,
-			   NULL, done_label, PROB_EVEN);
-  do_compare_rtx_and_jump (v, v2, EQ, false, v_mode, NULL_RTX,
-			   NULL, done_label, PROB_EVEN);
-  emit_move_insn (target, const0_rtx);
-  emit_label (done_label);
-
-  return target;
-}
-
-
 /* Expand an expression EXP that calls a built-in function,
    with result going to TARGET if that's convenient
    (and in mode MODE if that's convenient).
@@ -7036,9 +6996,8 @@ expand_builtin (tree exp, rtx target, rt
       break;
 
     case BUILT_IN_ACC_ON_DEVICE:
-      target = expand_builtin_acc_on_device (exp, target);
-      if (target)
-	return target;
+      /* Do library call, if we failed to expand the builtin when
+	 folding.  */
       break;
 
     default:	/* just do library call, if unknown builtin */
@@ -10271,6 +10230,27 @@ fold_builtin_1 (location_t loc, tree fnd
 	return build_empty_stmt (loc);
       break;
 
+    case BUILT_IN_ACC_ON_DEVICE:
+      /* Don't fold on_device until we know which compiler is active.  */
+      if (symtab->state == EXPANSION)
+	{
+	  unsigned val_host = GOMP_DEVICE_HOST;
+	  unsigned val_dev = GOMP_DEVICE_NONE;
+
+#ifdef ACCEL_COMPILER
+	  val_host = GOMP_DEVICE_NOT_HOST;
+	  val_dev = ACCEL_COMPILER_acc_device;
+#endif
+	  tree host = build2 (EQ_EXPR, boolean_type_node, arg0,
+			      build_int_cst (integer_type_node, val_host));
+	  tree dev = build2 (EQ_EXPR, boolean_type_node, arg0,
+			     build_int_cst (integer_type_node, val_dev));
+
+	  tree result = build2 (TRUTH_OR_EXPR, boolean_type_node, host, dev);
+	  return fold_convert (integer_type_node, result);
+	}
+      break;
+
     default:
       break;
     }

Re: Fold acc_on_device

[Bernd Schmidt]

On 09/29/2015 08:21 PM, Nathan Sidwell wrote:
> This patch folds acc_on_device as a regular builtin, but postponed until
> we know which compiler we're in.  As suggested by Bernd, we use the
> existing builtin folding machinery.
>
> Trunk is still using  the older PTX runtime scheme (Thomas is working on
> that), so the only change there is in the  host-side libgomp piece.
>
> Ok for trunk?

Ok, although I really don't quite see the need to drop the expander.


Bernd

Re: Fold acc_on_device

[Nathan Sidwell]

On 09/29/15 15:52, Bernd Schmidt wrote:

> Ok, although I really don't quite see the need to drop the expander.

Unnecessary code duplication.  It's better to say something once in one place, 
than try and say it twice in two different places.

nathan

Re: Fold acc_on_device

[Richard Biener]

On Tue, Sep 29, 2015 at 8:21 PM, Nathan Sidwell <nathan@acm.org> wrote:
> This patch folds acc_on_device as a regular builtin, but postponed until we
> know which compiler we're in.  As suggested by Bernd, we use the existing
> builtin folding machinery.
>
> Trunk is still using  the older PTX runtime scheme (Thomas is working on
> that), so the only change there is in the  host-side libgomp piece.
>
> Ok for trunk?

Please don't add any new GENERIC based builtin folders.  Instead add to
gimple-fold.c:gimple_fold_builtin

Otherwise you're just generating more work for us who move foldings from
builtins.c to gimple-fold.c.

Thanks,
Richard.

> nathan

Re: Fold acc_on_device

[Nathan Sidwell]

On 09/30/15 04:07, Richard Biener wrote:
> On Tue, Sep 29, 2015 at 8:21 PM, Nathan Sidwell <nathan@acm.org> wrote:
>> This patch folds acc_on_device as a regular builtin, but postponed until we
>> know which compiler we're in.  As suggested by Bernd, we use the existing
>> builtin folding machinery.
>>
>> Trunk is still using  the older PTX runtime scheme (Thomas is working on
>> that), so the only change there is in the  host-side libgomp piece.
>>
>> Ok for trunk?
>
> Please don't add any new GENERIC based builtin folders.  Instead add to
> gimple-fold.c:gimple_fold_builtin
>
> Otherwise you're just generating more work for us who move foldings from
> builtins.c to gimple-fold.c.

Oh, sorry, I didn't know about that.  Will fix.

Should I use the same
  if (symtab->state == EXPANSION)
test to make sure we're after LTO read back (i.e. know which compiler we're in), 
or is there another way?

nathan

Re: Fold acc_on_device

[Bernd Schmidt]

On 09/30/2015 02:18 PM, Nathan Sidwell wrote:
> On 09/30/15 04:07, Richard Biener wrote:
>> Please don't add any new GENERIC based builtin folders.  Instead add to
>> gimple-fold.c:gimple_fold_builtin
>>
>> Otherwise you're just generating more work for us who move foldings from
>> builtins.c to gimple-fold.c.
>
> Oh, sorry, I didn't know about that.  Will fix.

Yeah. me neither - sorry about that. TBH if we're not supposed to add 
folders to builtins.c that could use a comment near the top of that file.


Bernd

Re: Fold acc_on_device

[Richard Biener]

On Wed, Sep 30, 2015 at 2:18 PM, Nathan Sidwell <nathan@acm.org> wrote:
> On 09/30/15 04:07, Richard Biener wrote:
>>
>> On Tue, Sep 29, 2015 at 8:21 PM, Nathan Sidwell <nathan@acm.org> wrote:
>>>
>>> This patch folds acc_on_device as a regular builtin, but postponed until
>>> we
>>> know which compiler we're in.  As suggested by Bernd, we use the existing
>>> builtin folding machinery.
>>>
>>> Trunk is still using  the older PTX runtime scheme (Thomas is working on
>>> that), so the only change there is in the  host-side libgomp piece.
>>>
>>> Ok for trunk?
>>
>>
>> Please don't add any new GENERIC based builtin folders.  Instead add to
>> gimple-fold.c:gimple_fold_builtin
>>
>> Otherwise you're just generating more work for us who move foldings from
>> builtins.c to gimple-fold.c.
>
>
> Oh, sorry, I didn't know about that.  Will fix.
>
> Should I use the same
>  if (symtab->state == EXPANSION)
> test to make sure we're after LTO read back (i.e. know which compiler we're
> in), or is there another way?

I don't know of a better way, no.  I'll add a comment to builtins.c
(not that I expect anyone sees it ;))

Richard.

>
> nathan
>

Re: Fold acc_on_device

[Nathan Sidwell]

On 09/30/15 08:46, Richard Biener wrote:

>  I'll add a comment to builtins.c
> (not that I expect anyone sees it ;))

Put one instance at the default: label in  expand_builtin?

nathan

Re: Fold acc_on_device

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 183 bytes --]

On 09/30/15 08:46, Richard Biener wrote:

>>> Please don't add any new GENERIC based builtin folders.  Instead add to
>>> gimple-fold.c:gimple_fold_builtin

Is this patch ok?

nathan

[-- Attachment #2: trunk-ondev-3.patch --]
[-- Type: text/x-patch, Size: 3337 bytes --]

2015-09-30  Nathan Sidwell  <nathan@codesourcery.com>

	* builtins.c: Don't include gomp-constants.h.
	(fold_builtin_1): Don't fold acc_on_device here.
	* gimple-fold.c: Include gomp-constants.h.
	(gimple_fold_builtin_acc_on_device): New.
	(gimple_fold_builtin): Call it.

Index: gimple-fold.c
===================================================================
--- gimple-fold.c	(revision 228288)
+++ gimple-fold.c	(working copy)
@@ -62,6 +62,7 @@ along with GCC; see the file COPYING3.
 #include "output.h"
 #include "tree-eh.h"
 #include "gimple-match.h"
+#include "gomp-constants.h"
 
 /* Return true when DECL can be referenced from current unit.
    FROM_DECL (if non-null) specify constructor of variable DECL was taken from.
@@ -2708,6 +2709,34 @@ gimple_fold_builtin_strlen (gimple_stmt_
   return true;
 }
 
+/* Fold a call to __builtin_acc_on_device.  */
+
+static bool
+gimple_fold_builtin_acc_on_device (gimple_stmt_iterator *gsi, tree arg0)
+{
+  /* Defer folding until we know which compiler we're in.  */
+  if (symtab->state != EXPANSION)
+    return false;
+
+  unsigned val_host = GOMP_DEVICE_HOST;
+  unsigned val_dev = GOMP_DEVICE_NONE;
+
+#ifdef ACCEL_COMPILER
+  val_host = GOMP_DEVICE_NOT_HOST;
+  val_dev = ACCEL_COMPILER_acc_device;
+#endif
+
+  tree host = build2 (EQ_EXPR, boolean_type_node, arg0,
+		      build_int_cst (integer_type_node, val_host));
+  tree dev = build2 (EQ_EXPR, boolean_type_node, arg0,
+		     build_int_cst (integer_type_node, val_dev));
+
+  tree result = build2 (TRUTH_OR_EXPR, boolean_type_node, host, dev);
+
+  result = fold_convert (integer_type_node, result);
+  gimplify_and_update_call_from_tree (gsi, result);
+  return true;
+}
 
 /* Fold the non-target builtin at *GSI and return whether any simplification
    was made.  */
@@ -2848,6 +2877,9 @@ gimple_fold_builtin (gimple_stmt_iterato
 					   n == 3
 					   ? gimple_call_arg (stmt, 2)
 					   : NULL_TREE, fcode);
+    case BUILT_IN_ACC_ON_DEVICE:
+      return gimple_fold_builtin_acc_on_device (gsi,
+						gimple_call_arg (stmt, 0));
     default:;
     }
 
Index: builtins.c
===================================================================
--- builtins.c	(revision 228288)
+++ builtins.c	(working copy)
@@ -64,7 +64,6 @@ along with GCC; see the file COPYING3.
 #include "cgraph.h"
 #include "tree-chkp.h"
 #include "rtl-chkp.h"
-#include "gomp-constants.h"
 
 
 static tree do_mpc_arg1 (tree, tree, int (*)(mpc_ptr, mpc_srcptr, mpc_rnd_t));
@@ -10230,27 +10229,6 @@ fold_builtin_1 (location_t loc, tree fnd
 	return build_empty_stmt (loc);
       break;
 
-    case BUILT_IN_ACC_ON_DEVICE:
-      /* Don't fold on_device until we know which compiler is active.  */
-      if (symtab->state == EXPANSION)
-	{
-	  unsigned val_host = GOMP_DEVICE_HOST;
-	  unsigned val_dev = GOMP_DEVICE_NONE;
-
-#ifdef ACCEL_COMPILER
-	  val_host = GOMP_DEVICE_NOT_HOST;
-	  val_dev = ACCEL_COMPILER_acc_device;
-#endif
-	  tree host = build2 (EQ_EXPR, boolean_type_node, arg0,
-			      build_int_cst (integer_type_node, val_host));
-	  tree dev = build2 (EQ_EXPR, boolean_type_node, arg0,
-			     build_int_cst (integer_type_node, val_dev));
-
-	  tree result = build2 (TRUTH_OR_EXPR, boolean_type_node, host, dev);
-	  return fold_convert (integer_type_node, result);
-	}
-      break;
-
     default:
       break;
     }

Re: Fold acc_on_device

[Jakub Jelinek]

On Wed, Sep 30, 2015 at 03:01:22PM -0400, Nathan Sidwell wrote:
> On 09/30/15 08:46, Richard Biener wrote:
> 
> >>>Please don't add any new GENERIC based builtin folders.  Instead add to
> >>>gimple-fold.c:gimple_fold_builtin
> 
> Is this patch ok?
> 
> nathan

> 2015-09-30  Nathan Sidwell  <nathan@codesourcery.com>
> 
> 	* builtins.c: Don't include gomp-constants.h.
> 	(fold_builtin_1): Don't fold acc_on_device here.
> 	* gimple-fold.c: Include gomp-constants.h.
> 	(gimple_fold_builtin_acc_on_device): New.
> 	(gimple_fold_builtin): Call it.
> 
> Index: gimple-fold.c
> ===================================================================
> --- gimple-fold.c	(revision 228288)
> +++ gimple-fold.c	(working copy)
> @@ -62,6 +62,7 @@ along with GCC; see the file COPYING3.
>  #include "output.h"
>  #include "tree-eh.h"
>  #include "gimple-match.h"
> +#include "gomp-constants.h"
>  
>  /* Return true when DECL can be referenced from current unit.
>     FROM_DECL (if non-null) specify constructor of variable DECL was taken from.
> @@ -2708,6 +2709,34 @@ gimple_fold_builtin_strlen (gimple_stmt_
>    return true;
>  }
>  
> +/* Fold a call to __builtin_acc_on_device.  */
> +
> +static bool
> +gimple_fold_builtin_acc_on_device (gimple_stmt_iterator *gsi, tree arg0)
> +{
> +  /* Defer folding until we know which compiler we're in.  */
> +  if (symtab->state != EXPANSION)
> +    return false;
> +
> +  unsigned val_host = GOMP_DEVICE_HOST;
> +  unsigned val_dev = GOMP_DEVICE_NONE;
> +
> +#ifdef ACCEL_COMPILER
> +  val_host = GOMP_DEVICE_NOT_HOST;
> +  val_dev = ACCEL_COMPILER_acc_device;
> +#endif
> +
> +  tree host = build2 (EQ_EXPR, boolean_type_node, arg0,
> +		      build_int_cst (integer_type_node, val_host));
> +  tree dev = build2 (EQ_EXPR, boolean_type_node, arg0,
> +		     build_int_cst (integer_type_node, val_dev));
> +
> +  tree result = build2 (TRUTH_OR_EXPR, boolean_type_node, host, dev);
> +
> +  result = fold_convert (integer_type_node, result);
> +  gimplify_and_update_call_from_tree (gsi, result);
> +  return true;
> +}

Wouldn't it be better to just emit GIMPLE here instead?
So
  tree res = make_ssa_name (boolean_type_node);
  gimple g = gimple_build_assign (res, EQ_EXPR, arg0,
				  build_int_cst (integer_type_node, val_host));
  gsi_insert_before (gsi, g);
...
?

	Jakub

Re: Fold acc_on_device

[Richard Biener]

On Wed, Sep 30, 2015 at 9:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Sep 30, 2015 at 03:01:22PM -0400, Nathan Sidwell wrote:
>> On 09/30/15 08:46, Richard Biener wrote:
>>
>> >>>Please don't add any new GENERIC based builtin folders.  Instead add to
>> >>>gimple-fold.c:gimple_fold_builtin
>>
>> Is this patch ok?
>>
>> nathan
>
>> 2015-09-30  Nathan Sidwell  <nathan@codesourcery.com>
>>
>>       * builtins.c: Don't include gomp-constants.h.
>>       (fold_builtin_1): Don't fold acc_on_device here.
>>       * gimple-fold.c: Include gomp-constants.h.
>>       (gimple_fold_builtin_acc_on_device): New.
>>       (gimple_fold_builtin): Call it.
>>
>> Index: gimple-fold.c
>> ===================================================================
>> --- gimple-fold.c     (revision 228288)
>> +++ gimple-fold.c     (working copy)
>> @@ -62,6 +62,7 @@ along with GCC; see the file COPYING3.
>>  #include "output.h"
>>  #include "tree-eh.h"
>>  #include "gimple-match.h"
>> +#include "gomp-constants.h"
>>
>>  /* Return true when DECL can be referenced from current unit.
>>     FROM_DECL (if non-null) specify constructor of variable DECL was taken from.
>> @@ -2708,6 +2709,34 @@ gimple_fold_builtin_strlen (gimple_stmt_
>>    return true;
>>  }
>>
>> +/* Fold a call to __builtin_acc_on_device.  */
>> +
>> +static bool
>> +gimple_fold_builtin_acc_on_device (gimple_stmt_iterator *gsi, tree arg0)
>> +{
>> +  /* Defer folding until we know which compiler we're in.  */
>> +  if (symtab->state != EXPANSION)
>> +    return false;
>> +
>> +  unsigned val_host = GOMP_DEVICE_HOST;
>> +  unsigned val_dev = GOMP_DEVICE_NONE;
>> +
>> +#ifdef ACCEL_COMPILER
>> +  val_host = GOMP_DEVICE_NOT_HOST;
>> +  val_dev = ACCEL_COMPILER_acc_device;
>> +#endif
>> +
>> +  tree host = build2 (EQ_EXPR, boolean_type_node, arg0,
>> +                   build_int_cst (integer_type_node, val_host));
>> +  tree dev = build2 (EQ_EXPR, boolean_type_node, arg0,
>> +                  build_int_cst (integer_type_node, val_dev));
>> +
>> +  tree result = build2 (TRUTH_OR_EXPR, boolean_type_node, host, dev);
>> +
>> +  result = fold_convert (integer_type_node, result);
>> +  gimplify_and_update_call_from_tree (gsi, result);
>> +  return true;
>> +}
>
> Wouldn't it be better to just emit GIMPLE here instead?
> So
>   tree res = make_ssa_name (boolean_type_node);
>   gimple g = gimple_build_assign (res, EQ_EXPR, arg0,
>                                   build_int_cst (integer_type_node, val_host));
>   gsi_insert_before (gsi, g);
> ...
> ?

Yeah, best with using gimple_build which also canonicalizes/optimizes.

Richard.

>         Jakub

Re: Fold acc_on_device

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

On 10/01/15 06:03, Richard Biener wrote:
> On Wed, Sep 30, 2015 at 9:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:

>> Wouldn't it be better to just emit GIMPLE here instead?
>> So
>>    tree res = make_ssa_name (boolean_type_node);
>>    gimple g = gimple_build_assign (res, EQ_EXPR, arg0,
>>                                    build_int_cst (integer_type_node, val_host));
>>    gsi_insert_before (gsi, g);
>> ...

Like this?

nathan

[-- Attachment #2: trunk-ondev-4.patch --]
[-- Type: text/x-patch, Size: 3827 bytes --]

2015-10-01  Nathan Sidwell  <nathan@codesourcery.com>

	* builtins.c: Don't include gomp-constants.h.
	(fold_builtin_1): Don't fold acc_on_device here.
	* gimple-fold.c: Include gomp-constants.h.
	(gimple_fold_builtin_acc_on_device): New.
	(gimple_fold_builtin): Call it.

Index: gcc/gimple-fold.c
===================================================================
--- gcc/gimple-fold.c	(revision 228288)
+++ gcc/gimple-fold.c	(working copy)
@@ -62,6 +62,7 @@ along with GCC; see the file COPYING3.
 #include "output.h"
 #include "tree-eh.h"
 #include "gimple-match.h"
+#include "gomp-constants.h"
 
 /* Return true when DECL can be referenced from current unit.
    FROM_DECL (if non-null) specify constructor of variable DECL was taken from.
@@ -2708,6 +2709,47 @@ gimple_fold_builtin_strlen (gimple_stmt_
   return true;
 }
 
+/* Fold a call to __builtin_acc_on_device.  */
+
+static bool
+gimple_fold_builtin_acc_on_device (gimple_stmt_iterator *gsi, tree arg0)
+{
+  /* Defer folding until we know which compiler we're in.  */
+  if (symtab->state != EXPANSION)
+    return false;
+
+  unsigned val_host = GOMP_DEVICE_HOST;
+  unsigned val_dev = GOMP_DEVICE_NONE;
+
+#ifdef ACCEL_COMPILER
+  val_host = GOMP_DEVICE_NOT_HOST;
+  val_dev = ACCEL_COMPILER_acc_device;
+#endif
+
+  location_t loc = gimple_location (gsi_stmt (*gsi));
+  
+  tree host_eq = make_ssa_name (boolean_type_node);
+  gimple *host_ass = gimple_build_assign
+    (host_eq, EQ_EXPR, arg0, build_int_cst (integer_type_node, val_host));
+  gimple_set_location (host_ass, loc);
+  gsi_insert_before (gsi, host_ass, GSI_SAME_STMT);
+
+  tree dev_eq = make_ssa_name (boolean_type_node);
+  gimple *dev_ass = gimple_build_assign
+    (dev_eq, EQ_EXPR, arg0, build_int_cst (integer_type_node, val_dev));
+  gimple_set_location (host_ass, loc);
+  gsi_insert_before (gsi, dev_ass, GSI_SAME_STMT);
+
+  tree result = make_ssa_name (boolean_type_node);
+  gimple *result_ass = gimple_build_assign
+    (result, BIT_IOR_EXPR, host_eq, dev_eq);
+  gimple_set_location (host_ass, loc);
+  gsi_insert_before (gsi, result_ass, GSI_SAME_STMT);
+
+  replace_call_with_value (gsi, result);
+
+  return true;
+}
 
 /* Fold the non-target builtin at *GSI and return whether any simplification
    was made.  */
@@ -2848,6 +2890,9 @@ gimple_fold_builtin (gimple_stmt_iterato
 					   n == 3
 					   ? gimple_call_arg (stmt, 2)
 					   : NULL_TREE, fcode);
+    case BUILT_IN_ACC_ON_DEVICE:
+      return gimple_fold_builtin_acc_on_device (gsi,
+						gimple_call_arg (stmt, 0));
     default:;
     }
 
Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	(revision 228288)
+++ gcc/builtins.c	(working copy)
@@ -64,7 +64,6 @@ along with GCC; see the file COPYING3.
 #include "cgraph.h"
 #include "tree-chkp.h"
 #include "rtl-chkp.h"
-#include "gomp-constants.h"
 
 
 static tree do_mpc_arg1 (tree, tree, int (*)(mpc_ptr, mpc_srcptr, mpc_rnd_t));
@@ -10230,27 +10229,6 @@ fold_builtin_1 (location_t loc, tree fnd
 	return build_empty_stmt (loc);
       break;
 
-    case BUILT_IN_ACC_ON_DEVICE:
-      /* Don't fold on_device until we know which compiler is active.  */
-      if (symtab->state == EXPANSION)
-	{
-	  unsigned val_host = GOMP_DEVICE_HOST;
-	  unsigned val_dev = GOMP_DEVICE_NONE;
-
-#ifdef ACCEL_COMPILER
-	  val_host = GOMP_DEVICE_NOT_HOST;
-	  val_dev = ACCEL_COMPILER_acc_device;
-#endif
-	  tree host = build2 (EQ_EXPR, boolean_type_node, arg0,
-			      build_int_cst (integer_type_node, val_host));
-	  tree dev = build2 (EQ_EXPR, boolean_type_node, arg0,
-			     build_int_cst (integer_type_node, val_dev));
-
-	  tree result = build2 (TRUTH_OR_EXPR, boolean_type_node, host, dev);
-	  return fold_convert (integer_type_node, result);
-	}
-      break;
-
     default:
       break;
     }

Re: Fold acc_on_device

[Richard Biener]

On Thu, Oct 1, 2015 at 2:33 PM, Nathan Sidwell <nathan@acm.org> wrote:
> On 10/01/15 06:03, Richard Biener wrote:
>>
>> On Wed, Sep 30, 2015 at 9:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>
>
>>> Wouldn't it be better to just emit GIMPLE here instead?
>>> So
>>>    tree res = make_ssa_name (boolean_type_node);
>>>    gimple g = gimple_build_assign (res, EQ_EXPR, arg0,
>>>                                    build_int_cst (integer_type_node,
>>> val_host));
>>>    gsi_insert_before (gsi, g);
>>> ...
>
>
> Like this?

+  gimple *host_ass = gimple_build_assign
+    (host_eq, EQ_EXPR, arg0, build_int_cst (integer_type_node, val_host));

use TREE_TYPE (arg0) for the integer cst.

Otherwise looks good to me.

Thanks,
Richard.

> nathan

Re: Fold acc_on_device

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 293 bytes --]

On 10/01/15 08:46, Richard Biener wrote:
> On Thu, Oct 1, 2015 at 2:33 PM, Nathan Sidwell <nathan@acm.org> wrote:

> use TREE_TYPE (arg0) for the integer cst.
>
> Otherwise looks good to me.
thanks,

fixed up and applied (also noticed a copy & paste malfunction setting the location)


nathan

[-- Attachment #2: trunk-ondev-5.patch --]
[-- Type: text/x-patch, Size: 3826 bytes --]

2015-10-01  Nathan Sidwell  <nathan@codesourcery.com>

	* builtins.c: Don't include gomp-constants.h.
	(fold_builtin_1): Don't fold acc_on_device here.
	* gimple-fold.c: Include gomp-constants.h.
	(gimple_fold_builtin_acc_on_device): New.
	(gimple_fold_builtin): Call it.

Index: gcc/gimple-fold.c
===================================================================
--- gcc/gimple-fold.c	(revision 228288)
+++ gcc/gimple-fold.c	(working copy)
@@ -62,6 +62,7 @@ along with GCC; see the file COPYING3.
 #include "output.h"
 #include "tree-eh.h"
 #include "gimple-match.h"
+#include "gomp-constants.h"
 
 /* Return true when DECL can be referenced from current unit.
    FROM_DECL (if non-null) specify constructor of variable DECL was taken from.
@@ -2708,6 +2709,47 @@ gimple_fold_builtin_strlen (gimple_stmt_
   return true;
 }
 
+/* Fold a call to __builtin_acc_on_device.  */
+
+static bool
+gimple_fold_builtin_acc_on_device (gimple_stmt_iterator *gsi, tree arg0)
+{
+  /* Defer folding until we know which compiler we're in.  */
+  if (symtab->state != EXPANSION)
+    return false;
+
+  unsigned val_host = GOMP_DEVICE_HOST;
+  unsigned val_dev = GOMP_DEVICE_NONE;
+
+#ifdef ACCEL_COMPILER
+  val_host = GOMP_DEVICE_NOT_HOST;
+  val_dev = ACCEL_COMPILER_acc_device;
+#endif
+
+  location_t loc = gimple_location (gsi_stmt (*gsi));
+  
+  tree host_eq = make_ssa_name (boolean_type_node);
+  gimple *host_ass = gimple_build_assign
+    (host_eq, EQ_EXPR, arg0, build_int_cst (TREE_TYPE (arg0), val_host));
+  gimple_set_location (host_ass, loc);
+  gsi_insert_before (gsi, host_ass, GSI_SAME_STMT);
+
+  tree dev_eq = make_ssa_name (boolean_type_node);
+  gimple *dev_ass = gimple_build_assign
+    (dev_eq, EQ_EXPR, arg0, build_int_cst (TREE_TYPE (arg0), val_dev));
+  gimple_set_location (dev_ass, loc);
+  gsi_insert_before (gsi, dev_ass, GSI_SAME_STMT);
+
+  tree result = make_ssa_name (boolean_type_node);
+  gimple *result_ass = gimple_build_assign
+    (result, BIT_IOR_EXPR, host_eq, dev_eq);
+  gimple_set_location (result_ass, loc);
+  gsi_insert_before (gsi, result_ass, GSI_SAME_STMT);
+
+  replace_call_with_value (gsi, result);
+
+  return true;
+}
 
 /* Fold the non-target builtin at *GSI and return whether any simplification
    was made.  */
@@ -2848,6 +2890,9 @@ gimple_fold_builtin (gimple_stmt_iterato
 					   n == 3
 					   ? gimple_call_arg (stmt, 2)
 					   : NULL_TREE, fcode);
+    case BUILT_IN_ACC_ON_DEVICE:
+      return gimple_fold_builtin_acc_on_device (gsi,
+						gimple_call_arg (stmt, 0));
     default:;
     }
 
Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	(revision 228288)
+++ gcc/builtins.c	(working copy)
@@ -64,7 +64,6 @@ along with GCC; see the file COPYING3.
 #include "cgraph.h"
 #include "tree-chkp.h"
 #include "rtl-chkp.h"
-#include "gomp-constants.h"
 
 
 static tree do_mpc_arg1 (tree, tree, int (*)(mpc_ptr, mpc_srcptr, mpc_rnd_t));
@@ -10230,27 +10229,6 @@ fold_builtin_1 (location_t loc, tree fnd
 	return build_empty_stmt (loc);
       break;
 
-    case BUILT_IN_ACC_ON_DEVICE:
-      /* Don't fold on_device until we know which compiler is active.  */
-      if (symtab->state == EXPANSION)
-	{
-	  unsigned val_host = GOMP_DEVICE_HOST;
-	  unsigned val_dev = GOMP_DEVICE_NONE;
-
-#ifdef ACCEL_COMPILER
-	  val_host = GOMP_DEVICE_NOT_HOST;
-	  val_dev = ACCEL_COMPILER_acc_device;
-#endif
-	  tree host = build2 (EQ_EXPR, boolean_type_node, arg0,
-			      build_int_cst (integer_type_node, val_host));
-	  tree dev = build2 (EQ_EXPR, boolean_type_node, arg0,
-			     build_int_cst (integer_type_node, val_dev));
-
-	  tree result = build2 (TRUTH_OR_EXPR, boolean_type_node, host, dev);
-	  return fold_convert (integer_type_node, result);
-	}
-      break;
-
     default:
       break;
     }

Re: Fold acc_on_device

[Segher Boessenkool]

On Thu, Oct 01, 2015 at 08:33:07AM -0400, Nathan Sidwell wrote:
> 2015-10-01  Nathan Sidwell  <nathan@codesourcery.com>
> 
> 	* builtins.c: Don't include gomp-constants.h.
> 	(fold_builtin_1): Don't fold acc_on_device here.
> 	* gimple-fold.c: Include gomp-constants.h.
> 	(gimple_fold_builtin_acc_on_device): New.
> 	(gimple_fold_builtin): Call it.
> 
> Index: gcc/gimple-fold.c
> ===================================================================
> --- gcc/gimple-fold.c	(revision 228288)
> +++ gcc/gimple-fold.c	(working copy)
> @@ -2848,6 +2890,9 @@ gimple_fold_builtin (gimple_stmt_iterato
>  					   n == 3
>  					   ? gimple_call_arg (stmt, 2)
>  					   : NULL_TREE, fcode);

Missing break statement here.  This is PR67861.

> +    case BUILT_IN_ACC_ON_DEVICE:
> +      return gimple_fold_builtin_acc_on_device (gsi,
> +						gimple_call_arg (stmt, 0));
>      default:;
>      }


Segher

Re: Fold acc_on_device

[Nathan Sidwell]

On 10/06/15 02:12, Segher Boessenkool wrote:
> On Thu, Oct 01, 2015 at 08:33:07AM -0400, Nathan Sidwell wrote:
>> 2015-10-01  Nathan Sidwell  <nathan@codesourcery.com>
>>
>> 	* builtins.c: Don't include gomp-constants.h.
>> 	(fold_builtin_1): Don't fold acc_on_device here.
>> 	* gimple-fold.c: Include gomp-constants.h.
>> 	(gimple_fold_builtin_acc_on_device): New.
>> 	(gimple_fold_builtin): Call it.
>>
>> Index: gcc/gimple-fold.c
>> ===================================================================
>> --- gcc/gimple-fold.c	(revision 228288)
>> +++ gcc/gimple-fold.c	(working copy)
>> @@ -2848,6 +2890,9 @@ gimple_fold_builtin (gimple_stmt_iterato
>>   					   n == 3
>>   					   ? gimple_call_arg (stmt, 2)
>>   					   : NULL_TREE, fcode);
>
> Missing break statement here.  This is PR67861.

Thanks for clue bat.  testing now.

nathan

Re: Fold acc_on_device

[Andrew MacLeod]

On 09/30/2015 08:46 AM, Richard Biener wrote:
> On Wed, Sep 30, 2015 at 2:18 PM, Nathan Sidwell <nathan@acm.org> wrote:
>
> Please don't add any new GENERIC based builtin folders.  Instead add to
> gimple-fold.c:gimple_fold_builtin
>
> Otherwise you're just generating more work for us who move foldings from
> builtins.c to gimple-fold.c.
>>
>> Oh, sorry, I didn't know about that.  Will fix.
>>
>> Should I use the same
>>   if (symtab->state == EXPANSION)
>> test to make sure we're after LTO read back (i.e. know which compiler we're
>> in), or is there another way?
> I don't know of a better way, no.  I'll add a comment to builtins.c
> (not that I expect anyone sees it ;))
>
>

btw, not that it's necessarily important, but I'm about to submit the 
include reduction patches today,  and it turns out this line is the 
first use of anything from cgraph.h in builtins.c.

So if this is "the way" of doing the test, be aware it adds a dependency 
on cgraph.h that wasn't there before.

I noticed because the reducer finished on a 9/28 branch .   When I 
re-applied the patch to a 9/30 branch,  builtins.c failed to compile 
because it wasn't including cgraph.h for 'symtab' and 'EXPANSION'. So it 
wasn't required in 9/28.

Going forward, it will be obvious when we are adding a new dependency 
because the file will fail to compile without adding the requisite 
header file.

Andrew

Re: Fold acc_on_device

[Nathan Sidwell]

On 10/01/15 13:00, Andrew MacLeod wrote:

> btw, not that it's necessarily important, but I'm about to submit the include
> reduction patches today,  and it turns out this line is the first use of
> anything from cgraph.h in builtins.c.
>
> So if this is "the way" of doing the test, be aware it adds a dependency on
> cgraph.h that wasn't there before.

The patch I just committed has moved this to gimple-fold.c.  That appears to 
already include cgraph.h.

nathan

Re: Fold acc_on_device

[Andrew MacLeod]

On 10/01/2015 01:11 PM, Nathan Sidwell wrote:
> On 10/01/15 13:00, Andrew MacLeod wrote:
>
>> btw, not that it's necessarily important, but I'm about to submit the 
>> include
>> reduction patches today,  and it turns out this line is the first use of
>> anything from cgraph.h in builtins.c.
>>
>> So if this is "the way" of doing the test, be aware it adds a 
>> dependency on
>> cgraph.h that wasn't there before.
>
> The patch I just committed has moved this to gimple-fold.c.  That 
> appears to already include cgraph.h.
>
>
And it does indeed remove the need for cgraph.h in builtins.c.

Andrew