| Message ID | CAAgBjMnmmAZ1=PkPnTpt1PHtX5WiXongK=PoMKK7aFMXTcjvHQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 7 Feb 2017, Prathamesh Kulkarni wrote: > Hi Richard, > The attached patch tries to handle ABS_EXPR in gimple-fe. > I am not sure if __ABS_EXPR should be parsed as id (like __MEM) > or parse it as keyword (like __GIMPLE). Currently in the patch, I > parse it as id. > Patch passes gimplefe tests, is it OK to commit after bootstrap+test ? Few comments - the new oper_code argument to unary parsing seems superfluous - you check the name again for __ABS_EXPR. Also I'd spell it __ABS, without the _EXPR part. For __MEM we go to binary op parsing and then take the "not binary op" route. I guess some refactoring might clean up things here - not for now though. I'm not sure whether new RID_ keywords would be prefered for this kind of stuff. We added one for __PHI. Joseph, is the RID_ space somehow limited so that we should avoid ending up with, say, up to 226 RID_s for GIMPLE (upper estimate taken from the number of tree codes we have in tree.def)? I see currently cpplib puts rid_code into an unsigned char and we do already seem to have quite some of them (114 if I count correctly). Thanks, Richard.
On Tue, 7 Feb 2017, Richard Biener wrote: > I'm not sure whether new RID_ keywords would be prefered for this > kind of stuff. We added one for __PHI. Joseph, is the RID_ space > somehow limited so that we should avoid ending up with, say, up to > 226 RID_s for GIMPLE (upper estimate taken from the number of > tree codes we have in tree.def)? I see currently cpplib puts > rid_code into an unsigned char and we do already seem to have quite > some of them (114 if I count correctly). Apart from unsigned char, various places use 8-bit bit-fields for these values (the C and C++ parsers and c-indentation.h, at least). If necessary there would probably be scope for expanding those bit-fields without making tokens take more space, but I'm not sure about finding such space in cpp_hashnode and of course you'd need to find everywhere that uses unsigned char or a <= 8-bit bit-field for this. -- Joseph S. Myers joseph@codesourcery.com
On 7 February 2017 at 20:06, Richard Biener <rguenther@suse.de> wrote: > On Tue, 7 Feb 2017, Prathamesh Kulkarni wrote: > >> Hi Richard, >> The attached patch tries to handle ABS_EXPR in gimple-fe. >> I am not sure if __ABS_EXPR should be parsed as id (like __MEM) >> or parse it as keyword (like __GIMPLE). Currently in the patch, I >> parse it as id. >> Patch passes gimplefe tests, is it OK to commit after bootstrap+test ? > > Few comments - the new oper_code argument to unary parsing seems > superfluous - you check the name again for __ABS_EXPR. Also I'd > spell it __ABS, without the _EXPR part. Thanks for the suggestions. The name is not checked again for __ABS_EXPR, if oper_code is set to sth other than ERROR_MARK. oper_code is set only by c_parser_gimple_statement. However I agree it's ugly since the comparison code is placed in both the functions. In the attached patch, I changed it to __ABS and name comparison is done only within c_parser_gimple_unary_expression. However it uses an ugly hack to know it's called from c_parser_gimple_statement and then backs off from further parsing the token if it's type is CPP_NAME and value is not "__ABS". Not sure if this is good idea either. Thanks, Prathamesh > > For __MEM we go to binary op parsing and then take the "not binary op" > route. I guess some refactoring might clean up things here - not for > now though. > > I'm not sure whether new RID_ keywords would be prefered for this > kind of stuff. We added one for __PHI. Joseph, is the RID_ space > somehow limited so that we should avoid ending up with, say, up to > 226 RID_s for GIMPLE (upper estimate taken from the number of > tree codes we have in tree.def)? I see currently cpplib puts > rid_code into an unsigned char and we do already seem to have quite > some of them (114 if I count correctly). > > Thanks, > Richard.diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c index e167e42..942597a 100644 --- a/gcc/c/gimple-parser.c +++ b/gcc/c/gimple-parser.c @@ -61,7 +61,7 @@ static bool c_parser_gimple_compound_statement (c_parser *, gimple_seq *); static void c_parser_gimple_label (c_parser *, gimple_seq *); static void c_parser_gimple_statement (c_parser *, gimple_seq *); static struct c_expr c_parser_gimple_binary_expression (c_parser *); -static struct c_expr c_parser_gimple_unary_expression (c_parser *); +static struct c_expr c_parser_gimple_unary_expression (c_parser *, bool called_from_stmt_parser = false); static struct c_expr c_parser_gimple_postfix_expression (c_parser *); static struct c_expr c_parser_gimple_postfix_expression_after_primary (c_parser *, location_t, @@ -323,7 +323,8 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq) } /* Unary expression. */ - switch (c_parser_peek_token (parser)->type) + enum cpp_ttype toktype = c_parser_peek_token (parser)->type; + switch (toktype) { case CPP_KEYWORD: if (c_parser_peek_token (parser)->keyword != RID_REALPART @@ -336,7 +337,12 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq) case CPP_COMPL: case CPP_NOT: case CPP_MULT: /* pointer deref */ - rhs = c_parser_gimple_unary_expression (parser); + case CPP_NAME: + rhs = c_parser_gimple_unary_expression (parser, true); + if (toktype == CPP_NAME + && rhs.value == NULL) + break; + if (rhs.value != error_mark_node) { assign = gimple_build_assign (lhs.value, rhs.value); @@ -537,11 +543,11 @@ c_parser_gimple_binary_expression (c_parser *parser) unary-operator gimple-postfix-expression unary-operator: one of - & * + - ~ + & * + - ~ abs_expr */ static c_expr -c_parser_gimple_unary_expression (c_parser *parser) +c_parser_gimple_unary_expression (c_parser *parser, bool called_from_stmt_parser) { struct c_expr ret, op; location_t op_loc = c_parser_peek_token (parser)->location; @@ -599,6 +606,23 @@ c_parser_gimple_unary_expression (c_parser *parser) default: return c_parser_gimple_postfix_expression (parser); } + case CPP_NAME: + { + tree id = c_parser_peek_token (parser)->value; + if (strcmp (IDENTIFIER_POINTER (id), "__ABS") == 0) + { + c_parser_consume_token (parser); + op = c_parser_gimple_postfix_expression (parser); + return parser_build_unary_op (op_loc, ABS_EXPR, op); + } + else if (called_from_stmt_parser) + { + ret.value = NULL; + return ret; + } + else + return c_parser_gimple_postfix_expression (parser); + } default: return c_parser_gimple_postfix_expression (parser); } diff --git a/gcc/gimple-pretty-print.c b/gcc/gimple-pretty-print.c index 91c839d..0033e97 100644 --- a/gcc/gimple-pretty-print.c +++ b/gcc/gimple-pretty-print.c @@ -323,9 +323,17 @@ dump_unary_rhs (pretty_printer *buffer, gassign *gs, int spc, int flags) break; case ABS_EXPR: - pp_string (buffer, "ABS_EXPR <"); - dump_generic_node (buffer, rhs, spc, flags, false); - pp_greater (buffer); + if (flags & TDF_GIMPLE) + { + pp_string (buffer, "__ABS "); + dump_generic_node (buffer, rhs, spc, flags, false); + } + else + { + pp_string (buffer, "ABS_EXPR <"); + dump_generic_node (buffer, rhs, spc, flags, false); + pp_greater (buffer); + } break; default: diff --git a/gcc/testsuite/gcc.dg/gimplefe-25.c b/gcc/testsuite/gcc.dg/gimplefe-25.c new file mode 100644 index 0000000..c75ea67 --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-25.c @@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fgimple -fdump-tree-ssa-gimple" } */ + +int __GIMPLE foo(int a) +{ + int t1; + t1_1 = __ABS a; + return t1_1; +} + +/* { dg-final { scan-tree-dump "__ABS a" "ssa" } } */
On Wed, 8 Feb 2017, Prathamesh Kulkarni wrote: > On 7 February 2017 at 20:06, Richard Biener <rguenther@suse.de> wrote: > > On Tue, 7 Feb 2017, Prathamesh Kulkarni wrote: > > > >> Hi Richard, > >> The attached patch tries to handle ABS_EXPR in gimple-fe. > >> I am not sure if __ABS_EXPR should be parsed as id (like __MEM) > >> or parse it as keyword (like __GIMPLE). Currently in the patch, I > >> parse it as id. > >> Patch passes gimplefe tests, is it OK to commit after bootstrap+test ? > > > > Few comments - the new oper_code argument to unary parsing seems > > superfluous - you check the name again for __ABS_EXPR. Also I'd > > spell it __ABS, without the _EXPR part. > Thanks for the suggestions. The name is not checked again for > __ABS_EXPR, if oper_code is set to sth > other than ERROR_MARK. oper_code is set only by c_parser_gimple_statement. > However I agree it's ugly since the comparison code is placed in both > the functions. > > In the attached patch, I changed it to __ABS and name comparison is > done only within > c_parser_gimple_unary_expression. However it uses an ugly hack to know > it's called from > c_parser_gimple_statement and then backs off from further parsing the > token if it's type is > CPP_NAME and value is not "__ABS". Not sure if this is good idea either. I'd rather compare the name twice without any extra parameter passing. As said, some bigger refactoring should be done to avoid this in the future. Richard. > Thanks, > Prathamesh > > > > For __MEM we go to binary op parsing and then take the "not binary op" > > route. I guess some refactoring might clean up things here - not for > > now though. > > > > I'm not sure whether new RID_ keywords would be prefered for this > > kind of stuff. We added one for __PHI. Joseph, is the RID_ space > > somehow limited so that we should avoid ending up with, say, up to > > 226 RID_s for GIMPLE (upper estimate taken from the number of > > tree codes we have in tree.def)? I see currently cpplib puts > > rid_code into an unsigned char and we do already seem to have quite > > some of them (114 if I count correctly). > > > > Thanks, > > Richard. > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
On 8 February 2017 at 17:24, Richard Biener <rguenther@suse.de> wrote: > On Wed, 8 Feb 2017, Prathamesh Kulkarni wrote: > >> On 7 February 2017 at 20:06, Richard Biener <rguenther@suse.de> wrote: >> > On Tue, 7 Feb 2017, Prathamesh Kulkarni wrote: >> > >> >> Hi Richard, >> >> The attached patch tries to handle ABS_EXPR in gimple-fe. >> >> I am not sure if __ABS_EXPR should be parsed as id (like __MEM) >> >> or parse it as keyword (like __GIMPLE). Currently in the patch, I >> >> parse it as id. >> >> Patch passes gimplefe tests, is it OK to commit after bootstrap+test ? >> > >> > Few comments - the new oper_code argument to unary parsing seems >> > superfluous - you check the name again for __ABS_EXPR. Also I'd >> > spell it __ABS, without the _EXPR part. >> Thanks for the suggestions. The name is not checked again for >> __ABS_EXPR, if oper_code is set to sth >> other than ERROR_MARK. oper_code is set only by c_parser_gimple_statement. >> However I agree it's ugly since the comparison code is placed in both >> the functions. >> >> In the attached patch, I changed it to __ABS and name comparison is >> done only within >> c_parser_gimple_unary_expression. However it uses an ugly hack to know >> it's called from >> c_parser_gimple_statement and then backs off from further parsing the >> token if it's type is >> CPP_NAME and value is not "__ABS". Not sure if this is good idea either. > > I'd rather compare the name twice without any extra parameter passing. > As said, some bigger refactoring should be done to avoid this in the > future. Is this version OK after bootstrap+test ? Thanks, Prathamesh > > Richard. > >> Thanks, >> Prathamesh >> > >> > For __MEM we go to binary op parsing and then take the "not binary op" >> > route. I guess some refactoring might clean up things here - not for >> > now though. >> > >> > I'm not sure whether new RID_ keywords would be prefered for this >> > kind of stuff. We added one for __PHI. Joseph, is the RID_ space >> > somehow limited so that we should avoid ending up with, say, up to >> > 226 RID_s for GIMPLE (upper estimate taken from the number of >> > tree codes we have in tree.def)? I see currently cpplib puts >> > rid_code into an unsigned char and we do already seem to have quite >> > some of them (114 if I count correctly). >> > >> > Thanks, >> > Richard. >> > > -- > Richard Biener <rguenther@suse.de> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c index e167e42..681951c 100644 --- a/gcc/c/gimple-parser.c +++ b/gcc/c/gimple-parser.c @@ -325,6 +325,13 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq) /* Unary expression. */ switch (c_parser_peek_token (parser)->type) { + case CPP_NAME: + { + tree id = c_parser_peek_token (parser)->value; + if (strcmp (IDENTIFIER_POINTER (id), "__ABS") == 0) + goto build_unary_expr; + break; + } case CPP_KEYWORD: if (c_parser_peek_token (parser)->keyword != RID_REALPART && c_parser_peek_token (parser)->keyword != RID_IMAGPART) @@ -336,6 +343,7 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq) case CPP_COMPL: case CPP_NOT: case CPP_MULT: /* pointer deref */ + build_unary_expr: rhs = c_parser_gimple_unary_expression (parser); if (rhs.value != error_mark_node) { @@ -537,7 +545,7 @@ c_parser_gimple_binary_expression (c_parser *parser) unary-operator gimple-postfix-expression unary-operator: one of - & * + - ~ + & * + - ~ abs_expr */ static c_expr @@ -599,6 +607,18 @@ c_parser_gimple_unary_expression (c_parser *parser) default: return c_parser_gimple_postfix_expression (parser); } + case CPP_NAME: + { + tree id = c_parser_peek_token (parser)->value; + if (strcmp (IDENTIFIER_POINTER (id), "__ABS") == 0) + { + c_parser_consume_token (parser); + op = c_parser_gimple_postfix_expression (parser); + return parser_build_unary_op (op_loc, ABS_EXPR, op); + } + else + return c_parser_gimple_postfix_expression (parser); + } default: return c_parser_gimple_postfix_expression (parser); } diff --git a/gcc/gimple-pretty-print.c b/gcc/gimple-pretty-print.c index 91c839d..0033e97 100644 --- a/gcc/gimple-pretty-print.c +++ b/gcc/gimple-pretty-print.c @@ -323,9 +323,17 @@ dump_unary_rhs (pretty_printer *buffer, gassign *gs, int spc, int flags) break; case ABS_EXPR: - pp_string (buffer, "ABS_EXPR <"); - dump_generic_node (buffer, rhs, spc, flags, false); - pp_greater (buffer); + if (flags & TDF_GIMPLE) + { + pp_string (buffer, "__ABS "); + dump_generic_node (buffer, rhs, spc, flags, false); + } + else + { + pp_string (buffer, "ABS_EXPR <"); + dump_generic_node (buffer, rhs, spc, flags, false); + pp_greater (buffer); + } break; default: diff --git a/gcc/testsuite/gcc.dg/gimplefe-25.c b/gcc/testsuite/gcc.dg/gimplefe-25.c new file mode 100644 index 0000000..d8c44a7 --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-25.c @@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-options "-O -fgimple -fdump-tree-ssa-gimple" } */ + +int __GIMPLE() f(int a) +{ + int t0; + t0_1 = __ABS a; + return t0_1; +} + +/* { dg-final { scan-tree-dump "__ABS a" "ssa" } } */
On Wed, 8 Feb 2017, Prathamesh Kulkarni wrote: > On 8 February 2017 at 17:24, Richard Biener <rguenther@suse.de> wrote: > > On Wed, 8 Feb 2017, Prathamesh Kulkarni wrote: > > > >> On 7 February 2017 at 20:06, Richard Biener <rguenther@suse.de> wrote: > >> > On Tue, 7 Feb 2017, Prathamesh Kulkarni wrote: > >> > > >> >> Hi Richard, > >> >> The attached patch tries to handle ABS_EXPR in gimple-fe. > >> >> I am not sure if __ABS_EXPR should be parsed as id (like __MEM) > >> >> or parse it as keyword (like __GIMPLE). Currently in the patch, I > >> >> parse it as id. > >> >> Patch passes gimplefe tests, is it OK to commit after bootstrap+test ? > >> > > >> > Few comments - the new oper_code argument to unary parsing seems > >> > superfluous - you check the name again for __ABS_EXPR. Also I'd > >> > spell it __ABS, without the _EXPR part. > >> Thanks for the suggestions. The name is not checked again for > >> __ABS_EXPR, if oper_code is set to sth > >> other than ERROR_MARK. oper_code is set only by c_parser_gimple_statement. > >> However I agree it's ugly since the comparison code is placed in both > >> the functions. > >> > >> In the attached patch, I changed it to __ABS and name comparison is > >> done only within > >> c_parser_gimple_unary_expression. However it uses an ugly hack to know > >> it's called from > >> c_parser_gimple_statement and then backs off from further parsing the > >> token if it's type is > >> CPP_NAME and value is not "__ABS". Not sure if this is good idea either. > > > > I'd rather compare the name twice without any extra parameter passing. > > As said, some bigger refactoring should be done to avoid this in the > > future. > Is this version OK after bootstrap+test ? Yes. Thanks, Richard. > Thanks, > Prathamesh > > > > Richard. > > > >> Thanks, > >> Prathamesh > >> > > >> > For __MEM we go to binary op parsing and then take the "not binary op" > >> > route. I guess some refactoring might clean up things here - not for > >> > now though. > >> > > >> > I'm not sure whether new RID_ keywords would be prefered for this > >> > kind of stuff. We added one for __PHI. Joseph, is the RID_ space > >> > somehow limited so that we should avoid ending up with, say, up to > >> > 226 RID_s for GIMPLE (upper estimate taken from the number of > >> > tree codes we have in tree.def)? I see currently cpplib puts > >> > rid_code into an unsigned char and we do already seem to have quite > >> > some of them (114 if I count correctly). > >> > > >> > Thanks, > >> > Richard. > >> > > > > -- > > Richard Biener <rguenther@suse.de> > > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c index e167e42..51bbfdc 100644 --- a/gcc/c/gimple-parser.c +++ b/gcc/c/gimple-parser.c @@ -61,7 +61,7 @@ static bool c_parser_gimple_compound_statement (c_parser *, gimple_seq *); static void c_parser_gimple_label (c_parser *, gimple_seq *); static void c_parser_gimple_statement (c_parser *, gimple_seq *); static struct c_expr c_parser_gimple_binary_expression (c_parser *); -static struct c_expr c_parser_gimple_unary_expression (c_parser *); +static struct c_expr c_parser_gimple_unary_expression (c_parser *, enum tree_code oper_code = ERROR_MARK); static struct c_expr c_parser_gimple_postfix_expression (c_parser *); static struct c_expr c_parser_gimple_postfix_expression_after_primary (c_parser *, location_t, @@ -323,8 +323,19 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq) } /* Unary expression. */ + enum tree_code oper_code = ERROR_MARK; switch (c_parser_peek_token (parser)->type) { + case CPP_NAME: + { + tree id = c_parser_peek_token (parser)->value; + if (strcmp (IDENTIFIER_POINTER (id), "__ABS_EXPR") == 0) + { + oper_code = ABS_EXPR; + goto build_unary_expr; + } + break; + } case CPP_KEYWORD: if (c_parser_peek_token (parser)->keyword != RID_REALPART && c_parser_peek_token (parser)->keyword != RID_IMAGPART) @@ -336,7 +347,8 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq) case CPP_COMPL: case CPP_NOT: case CPP_MULT: /* pointer deref */ - rhs = c_parser_gimple_unary_expression (parser); + build_unary_expr: + rhs = c_parser_gimple_unary_expression (parser, oper_code); if (rhs.value != error_mark_node) { assign = gimple_build_assign (lhs.value, rhs.value); @@ -537,11 +549,11 @@ c_parser_gimple_binary_expression (c_parser *parser) unary-operator gimple-postfix-expression unary-operator: one of - & * + - ~ + & * + - ~ abs_expr */ static c_expr -c_parser_gimple_unary_expression (c_parser *parser) +c_parser_gimple_unary_expression (c_parser *parser, enum tree_code oper_code) { struct c_expr ret, op; location_t op_loc = c_parser_peek_token (parser)->location; @@ -599,6 +611,25 @@ c_parser_gimple_unary_expression (c_parser *parser) default: return c_parser_gimple_postfix_expression (parser); } + case CPP_NAME: + { + if (oper_code != ERROR_MARK) + { + c_parser_consume_token (parser); + op = c_parser_gimple_postfix_expression (parser); + return parser_build_unary_op (op_loc, oper_code, op); + } + + tree id = c_parser_peek_token (parser)->value; + if (strcmp (IDENTIFIER_POINTER (id), "__ABS_EXPR") == 0) + { + c_parser_consume_token (parser); + op = c_parser_gimple_postfix_expression (parser); + return parser_build_unary_op (op_loc, ABS_EXPR, op); + } + else + return c_parser_gimple_postfix_expression (parser); + } default: return c_parser_gimple_postfix_expression (parser); } diff --git a/gcc/gimple-pretty-print.c b/gcc/gimple-pretty-print.c index 91c839d..6e9102e 100644 --- a/gcc/gimple-pretty-print.c +++ b/gcc/gimple-pretty-print.c @@ -323,9 +323,17 @@ dump_unary_rhs (pretty_printer *buffer, gassign *gs, int spc, int flags) break; case ABS_EXPR: - pp_string (buffer, "ABS_EXPR <"); - dump_generic_node (buffer, rhs, spc, flags, false); - pp_greater (buffer); + if (flags & TDF_GIMPLE) + { + pp_string (buffer, "__ABS_EXPR "); + dump_generic_node (buffer, rhs, spc, flags, false); + } + else + { + pp_string (buffer, "ABS_EXPR <"); + dump_generic_node (buffer, rhs, spc, flags, false); + pp_greater (buffer); + } break; default: