Re: [PATCH] Pretty-print GOACC_REDUCTION arguments

[Thomas Schwinge <thomas@codesourcery.com>]

Hi!

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 would print the actual "GOMP_DIM_GANG" instead of "gang" etc.,
to make it easier to see where these values are coming from.

(I do see that for "CODE", we can easily use a suitable "DEF" macro with
the "IFN_GOACC_REDUCTION_CODES" define -- not currently possible with the
"GOMP_DIM_*" constants.  That can of course be addressed later,
separately, if a Reviewer agrees to the proposed patch generally.)

> OK for trunk, if testing is ok?

> --- a/gcc/gimple-pretty-print.c
> +++ b/gcc/gimple-pretty-print.c
> @@ -765,6 +766,40 @@ 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))
> +	  {

Will need to add a "default" case:

    [...]/source-gcc/gcc/gimple-pretty-print.c:771:9: warning: enumeration value 'IFN_MASK_LOAD' not handled in switch [-Wswitch]
      switch (gimple_call_internal_fn (gs))
             ^
    [followed by many more]

> +	  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 ();        \
           ^~~~~~~~
    source-gcc/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-1.c:55:3: note: in expansion of macro 'check_reduction_op'
       check_reduction_op (int, ^, 0, array[i], num_gangs (ng) num_workers (nw)
       ^~~~~~~~~~~~~~~~~~

    (gdb) bt
    #0  fancy_abort (file=0x1659d70 "[...]/source-gcc/gcc/tree.c", line=6606, function=0x165e7f8 <tree_to_uhwi(tree_node const*)::__FUNCTION__> "tree_to_uhwi") at [...]/source-gcc/gcc/diagnostic.c:1487
    #1  0x0000000000edf053 in tree_to_uhwi (t=<optimized out>) at [...]/source-gcc/gcc/tree.c:6606
    #2  0x000000000096ca63 in dump_gimple_call_args (buffer=0x7fffffffc6c0, gs=0x7ffff66ad210, flags=0) at [...]/source-gcc/gcc/gimple-pretty-print.c:777
    #3  0x000000000096f7f0 in dump_gimple_call (buffer=0x7fffffffc6c0, gs=0x7ffff66ad210, spc=20, flags=0) at [...]/source-gcc/gcc/gimple-pretty-print.c:946
    [...]

This seems to be the "LEVEL" being "-1" -- probably meaning "not yet
decided"?  (Haven't looked that up.)

> +		  {
> +		  case GOMP_DIM_GANG:
> +		    pp_string (buffer, " /*gang*/");
> +		    break;
> +		  case GOMP_DIM_WORKER:
> +		    pp_string (buffer, " /*worker*/");
> +		    break;
> +		  case GOMP_DIM_VECTOR:
> +		    pp_string (buffer, " /*vector*/");
> +		    break;
> +		  default:
> +		    gcc_unreachable ();
> +		  }
> +		break;
> +	      case 4:
> +		{
> +		  enum tree_code rcode
> +		    = (enum tree_code)tree_to_uhwi (gimple_call_arg (gs, i));
> +		  pp_string (buffer, " /*");
> +		  pp_string (buffer, op_symbol_code (rcode));
> +		  pp_string (buffer, "*/");
> +		}
> +		break;

I take it, there is no canned function for printing the textual
representation of the tree_code "OP" ("case 4").

> +	      }
> +	  }
>      }
>  
>    if (gimple_call_va_arg_pack_p (gs))


Grüße
 Thomas