* [PATCH] Pretty-print GOACC_REDUCTION arguments
@ 2017-09-25 14:58 Tom de Vries
2017-09-27 13:47 ` Thomas Schwinge
0 siblings, 1 reply; 4+ messages in thread
From: Tom de Vries @ 2017-09-25 14:58 UTC (permalink / raw)
To: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 340 bytes --]
Hi,
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);
...
OK for trunk, if testing is ok?
Thanks,
- Tom
[-- Attachment #2: 0001-Pretty-print-GOACC_REDUCTION-arguments.patch --]
[-- Type: text/x-patch, Size: 1902 bytes --]
Pretty-print GOACC_REDUCTION arguments
Prints
sum_5 = GOACC_REDUCTION (SETUP, _3, 0, 0 /*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 | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/gcc/gimple-pretty-print.c b/gcc/gimple-pretty-print.c
index ed8e51c..61efd93 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,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))
+ {
+ case IFN_GOACC_REDUCTION:
+ switch (i)
+ {
+ case 3:
+ switch (tree_to_uhwi (gimple_call_arg (gs, i)))
+ {
+ 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;
+ }
+ }
}
if (gimple_call_va_arg_pack_p (gs))
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Pretty-print GOACC_REDUCTION arguments
2017-09-25 14:58 [PATCH] Pretty-print GOACC_REDUCTION arguments Tom de Vries
@ 2017-09-27 13:47 ` Thomas Schwinge
2017-10-03 8:57 ` Tom de Vries
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Schwinge @ 2017-09-27 13:47 UTC (permalink / raw)
To: Tom de Vries; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Pretty-print GOACC_REDUCTION arguments
2017-09-27 13:47 ` Thomas Schwinge
@ 2017-10-03 8:57 ` Tom de Vries
2017-10-04 15:01 ` Thomas Schwinge
0 siblings, 1 reply; 4+ messages in thread
From: Tom de Vries @ 2017-10-03 8:57 UTC (permalink / raw)
To: Thomas Schwinge; +Cc: GCC Patches, Jakub Jelinek
[-- Attachment #1: Type: text/plain, Size: 2738 bytes --]
On 09/27/2017 03:46 PM, Thomas Schwinge wrote:
> 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 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.
> I would print the actual "GOMP_DIM_GANG" instead of "gang" etc.,
> to make it easier to see where these values are coming from.
>
Done.
>> OK for trunk, if testing is ok?
>> + 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.)
>
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.
Bootstrapped and reg-tested on x86_64.
OK for trunk?
Thanks,
- Tom
[-- Attachment #2: 0001-Pretty-print-GOACC_REDUCTION-arguments.patch --]
[-- Type: text/x-patch, Size: 2075 bytes --]
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))
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Pretty-print GOACC_REDUCTION arguments
2017-10-03 8:57 ` Tom de Vries
@ 2017-10-04 15:01 ` Thomas Schwinge
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Schwinge @ 2017-10-04 15:01 UTC (permalink / raw)
To: Tom de Vries; +Cc: GCC Patches, Jakub Jelinek
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))
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-10-04 15:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-25 14:58 [PATCH] Pretty-print GOACC_REDUCTION arguments Tom de Vries
2017-09-27 13:47 ` Thomas Schwinge
2017-10-03 8:57 ` Tom de Vries
2017-10-04 15:01 ` Thomas Schwinge
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).