diff mbox

[GIMPLE,FE] Handle abs_expr

Message ID CAAgBjMmE9QdJ2EohAqtLBxSC3zk=cfSLF6M=6A8nQmBvG-TRBA@mail.gmail.com
State New
Headers show

Commit Message

Prathamesh Kulkarni Feb. 8, 2017, 12:14 p.m. UTC
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)

Comments

Richard Biener Feb. 8, 2017, 12:16 p.m. UTC | #1
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)
>
diff mbox

Patch

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" } } */