Re: [PATCH] Pretty-print GOACC_REDUCTION arguments

[Thomas Schwinge <thomas at codesourcery dot com>]

Hi Tom!

On Tue, 3 Oct 2017 10:56:59 +0200, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 09/27/2017 03:46 PM, Thomas Schwinge wrote:
> > On Mon, 25 Sep 2017 16:57:52 +0200, Tom de Vries <Tom_deVries@mentor.com> wrote:
> >> currently for a GOACC_REDUCTION internal fn call we print:
> >> ...
> >>     sum_5 = GOACC_REDUCTION (SETUP, _3, 0, 0, 67, 0);
> >> ...
> >>
> >> This patch adds a comment for some arguments explaining the meaning of
> >> the argument:
> >> ...
> >>         sum_5 = GOACC_REDUCTION (SETUP, _3, 0, 0 /*gang*/, 67 /*+*/, 0);
> >> ...
> > 
> > ACK to the general idea.
> > 
> > However, I note that for the "CODE" we currently *only* print the textual
> > variant ("SETUP") (just like we're also doing for a few other "IFN_*"),
> > whereas for "LEVEL" and "OP" you now print both.  Should these really be
> > different?  I think I actually do prefer the style you're using (print
> > both).
> 
> I chose the c-like syntax to make it easier to copy gimple to test-case 
> ( based on comment 
> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg02180.html ). But 
> reflecting on it a bit more, I realized that it's an internal function 
> and as such won't work anyway in test-cases, so I'm not sure how 
> relevant it is in this case.

The way you're doing it still makes sense to me: we might (at some later
point...) read such dumps back in, using the GIMPLE front end or similar.
(Clearly nothing to worry about right now.)


> >> +	  case IFN_GOACC_REDUCTION:
> >> +	    switch (i)
> >> +	      {
> >> +	      case 3:
> >> +		switch (tree_to_uhwi (gimple_call_arg (gs, i)))
> > 
> > Something ;-) is wrong.  Running this on
> > libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-1.c, I run into:
> > 
> >      during GIMPLE pass: omplower
> >      dump file: reduction-1.c.006t.omplower
> >      
> >      In file included from source-gcc/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-1.c:9:0:
> >      source-gcc/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-1.c: In function 'test_reductions':
> >      source-gcc/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction.h:20:7: internal compiler error: in tree_to_uhwi, at tree.c:6606
> >             abort ();        \
> 
> > This seems to be the "LEVEL" being "-1" -- probably meaning "not yet
> > decided"?  (Haven't looked that up.)

As far as I can tell, initially all these "LEVEL" arguments are set to
"-1".  See the "place" argument in the "lower_oacc_reductions" call in
"lower_oacc_head_tail".  So, I understand this to mean "not yet decided",
from here on, until some real level of parallelism gets set.

> In execute_oacc_device_lower we find:
> ...
>            case IFN_GOACC_REDUCTION:
>              /* Mark the function for SSA renaming.  */
> 	    mark_virtual_operands_for_renaming (cfun);
> 
>              /* If the level is -1, this ended up being an unused 
> 
>                 axis.  Handle as a default.  */
>              if (integer_minus_onep (gimple_call_arg (call, 3)))
>                default_goacc_reduction (call);
>              else
>                targetm.goacc.reduction (call);
> ...
> 
> I'm printing it now as GOMP_DIM_NONE.

But after this processing, there are no "IFN_GOACC_REDUCTION" anymore, so
we should rather print something like "unknown" instead of
"GOMP_DIM_NONE" (which doesn't exist anyway).


With that changed, we're good as far as I'm concerned, thanks!  But I
can't formally approve, of course.  Reviewed-by: Thomas Schwinge
<thomas@codesourcery.com> (See
<http://mid.mail-archive.com/87tvzuk29t.fsf@euler.schwinge.homeip.net>.)


Grüße
 Thomas


> Bootstrapped and reg-tested on x86_64.
> 
> OK for trunk?
> 
> Thanks,
> - Tom
> Pretty-print GOACC_REDUCTION arguments
> 
> Prints
>   sum_5 = GOACC_REDUCTION (SETUP, _3, 0, 0 /*GOMP_DIM_GANG*/, 67 /*+*/, 0);
> instead of
>   sum_5 = GOACC_REDUCTION (SETUP, _3, 0, 0, 67, 0);
> 
> 2017-09-25  Tom de Vries  <tom@codesourcery.com>
> 
> 	* gimple-pretty-print.c (dump_gimple_call_args): Pretty-print
> 	GOACC_REDUCTION arguments.
> 
> ---
>  gcc/gimple-pretty-print.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/gcc/gimple-pretty-print.c b/gcc/gimple-pretty-print.c
> index ed8e51c..33ca45d 100644
> --- a/gcc/gimple-pretty-print.c
> +++ b/gcc/gimple-pretty-print.c
> @@ -41,6 +41,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "stringpool.h"
>  #include "attribs.h"
>  #include "asan.h"
> +#include "gomp-constants.h"
>  
>  #define INDENT(SPACE)							\
>    do { int i; for (i = 0; i < SPACE; i++) pp_space (buffer); } while (0)
> @@ -765,6 +766,47 @@ dump_gimple_call_args (pretty_printer *buffer, gcall *gs, dump_flags_t flags)
>        if (i)
>  	pp_string (buffer, ", ");
>        dump_generic_node (buffer, gimple_call_arg (gs, i), 0, flags, false);
> +
> +      if (gimple_call_internal_p (gs))
> +	switch (gimple_call_internal_fn (gs))
> +	  {
> +	  case IFN_GOACC_REDUCTION:
> +	    switch (i)
> +	      {
> +	      case 3:
> +		switch (tree_to_shwi (gimple_call_arg (gs, i)))
> +		  {
> +		  case GOMP_DIM_GANG:
> +		    pp_string (buffer, " /*GOMP_DIM_GANG*/");
> +		    break;
> +		  case GOMP_DIM_WORKER:
> +		    pp_string (buffer, " /*GOMP_DIM_WORKER*/");
> +		    break;
> +		  case GOMP_DIM_VECTOR:
> +		    pp_string (buffer, " /*GOMP_DIM_VECTOR*/");
> +		    break;
> +		  case -1:
> +		    pp_string (buffer, " /*GOMP_DIM_NONE*/");
> +		    break;
> +		  default:
> +		    gcc_unreachable ();
> +		  }
> +		break;
> +	      case 4:
> +		{
> +		  enum tree_code rcode
> +		    = (enum tree_code)tree_to_shwi (gimple_call_arg (gs, i));
> +		  pp_string (buffer, " /*");
> +		  pp_string (buffer, op_symbol_code (rcode));
> +		  pp_string (buffer, "*/");
> +		}
> +		break;
> +	      default:
> +		break;
> +	      }
> +	  default:
> +	    break;
> +	  }
>      }
>  
>    if (gimple_call_va_arg_pack_p (gs))