diff mbox series

Fix gimple_expr_code?

Message ID 7dc11892-b595-68b2-7b28-5b16eec63941@redhat.com
State New
Headers show
Series Fix gimple_expr_code? | expand

Commit Message

Andrew MacLeod Nov. 12, 2020, 8:43 p.m. UTC 
So I spent some time tracking down a ranger issue, and in the end, it 
boiled down to the range-op handler not being picked up properly.

The handler is picked up by:

   if ((gimple_code (s) == GIMPLE_ASSIGN) || (gimple_code (s) == 
GIMPLE_COND))
     return range_op_handler (gimple_expr_code (s), gimple_expr_type (s));

where it is indexing the table with the gimple_expr_code..
the stmt being processed was for a pointer assignment,
   _5 = _33
and it was coming back with a gimple_expr_code of  VAR_DECL instead of 
an SSA_NAME... which confused me greatly.


gimple_expr_code (const gimple *stmt)
{
   enum gimple_code code = gimple_code (stmt);
   if (code == GIMPLE_ASSIGN || code == GIMPLE_COND)
     return (enum tree_code) stmt->subcode;

A little more digging shows this:

static inline enum tree_code
gimple_assign_rhs_code (const gassign *gs)
{
   enum tree_code code = (enum tree_code) gs->subcode;
   /* While we initially set subcode to the TREE_CODE of the rhs for
      GIMPLE_SINGLE_RHS assigns we do not update that subcode to stay
      in sync when we rewrite stmts into SSA form or do SSA 
propagations.  */
   if (get_gimple_rhs_class (code) == GIMPLE_SINGLE_RHS)
     code = TREE_CODE (gs->op[1]);

   return code;
}

Fascinating comment.

But it means that gimple_expr_code() isn't returning the correct result 
for GIMPLE_SINGLE_RHS....

Wouldn't it make sense that gimple_expr_code be changed to return 
gimple_assign_rhs_code() for GIMPLE_ASSIGN?

I tested the attached patch, and it bootstraps and passes regression tests.

There aren't a lot of places where its used, but I saw a suspicious bit 
in ipa-icf-gimple.c that looks like it is working around this?


    bool
    func_checker::compare_gimple_assign (gimple *s1, gimple *s2)
    {
       tree arg1, arg2;
       tree_code code1, code2;
       unsigned i;

       code1 = gimple_expr_code (s1);
       code2 = gimple_expr_code (s2);

       if (code1 != code2)
         return false;

       code1 = gimple_assign_rhs_code (s1);
       code2 = gimple_assign_rhs_code (s2);

       if (code1 != code2)
         return false;


and  there were one or two other places where SSA_NAME occurred in the 
cases of a switch after calling gimple_expr_code().

This seems like it should be the right thing?
Andrew

Comments

Richard Biener Nov. 12, 2020, 8:53 p.m. UTC | #1 
On November 12, 2020 9:43:52 PM GMT+01:00, Andrew MacLeod via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>So I spent some time tracking down a ranger issue, and in the end, it 
>boiled down to the range-op handler not being picked up properly.
>
>The handler is picked up by:
>
>   if ((gimple_code (s) == GIMPLE_ASSIGN) || (gimple_code (s) == 
>GIMPLE_COND))
>    return range_op_handler (gimple_expr_code (s), gimple_expr_type
>(s));

IMHO this should use more specific functions. Gimple_expr_code should go away similar to gimple_expr_type. 

>where it is indexing the table with the gimple_expr_code..
>the stmt being processed was for a pointer assignment,
>   _5 = _33
>and it was coming back with a gimple_expr_code of  VAR_DECL instead of 
>an SSA_NAME... which confused me greatly.
>
>
>gimple_expr_code (const gimple *stmt)
>{
>   enum gimple_code code = gimple_code (stmt);
>   if (code == GIMPLE_ASSIGN || code == GIMPLE_COND)
>     return (enum tree_code) stmt->subcode;
>
>A little more digging shows this:
>
>static inline enum tree_code
>gimple_assign_rhs_code (const gassign *gs)
>{
>   enum tree_code code = (enum tree_code) gs->subcode;
>   /* While we initially set subcode to the TREE_CODE of the rhs for
>      GIMPLE_SINGLE_RHS assigns we do not update that subcode to stay
>      in sync when we rewrite stmts into SSA form or do SSA 
>propagations.  */
>   if (get_gimple_rhs_class (code) == GIMPLE_SINGLE_RHS)
>     code = TREE_CODE (gs->op[1]);
>
>   return code;
>}
>
>Fascinating comment.

... 😬 

>But it means that gimple_expr_code() isn't returning the correct result
>
>for GIMPLE_SINGLE_RHS....

It depends. A SSA name isn't an expression code either. As said, the generic gimple_expr_code should be used with extreme care. 

>Wouldn't it make sense that gimple_expr_code be changed to return 
>gimple_assign_rhs_code() for GIMPLE_ASSIGN?
>
>I tested the attached patch, and it bootstraps and passes regression
>tests.
>
>There aren't a lot of places where its used, but I saw a suspicious bit
>
>in ipa-icf-gimple.c that looks like it is working around this?
>
>
>    bool
>    func_checker::compare_gimple_assign (gimple *s1, gimple *s2)
>    {
>       tree arg1, arg2;
>       tree_code code1, code2;
>       unsigned i;
>
>       code1 = gimple_expr_code (s1);
>       code2 = gimple_expr_code (s2);
>
>       if (code1 != code2)
>         return false;
>
>       code1 = gimple_assign_rhs_code (s1);
>       code2 = gimple_assign_rhs_code (s2);
>
>       if (code1 != code2)
>         return false;
>
>
>and  there were one or two other places where SSA_NAME occurred in the 
>cases of a switch after calling gimple_expr_code().
>
>This seems like it should be the right thing?
>Andrew
Andrew MacLeod Nov. 12, 2020, 9:12 p.m. UTC | #2 
On 11/12/20 3:53 PM, Richard Biener wrote:
> On November 12, 2020 9:43:52 PM GMT+01:00, Andrew MacLeod via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>> So I spent some time tracking down a ranger issue, and in the end, it
>> boiled down to the range-op handler not being picked up properly.
>>
>> The handler is picked up by:
>>
>>    if ((gimple_code (s) == GIMPLE_ASSIGN) || (gimple_code (s) ==
>> GIMPLE_COND))
>>      return range_op_handler (gimple_expr_code (s), gimple_expr_type
>> (s));
> IMHO this should use more specific functions. Gimple_expr_code should go away similar to gimple_expr_type.

gimple_expr_type is quite pervasive.. and each consumer is going to have 
to roll their own version of it.  Why do we want to get rid of it?

If we are trying to save a few bytes by storing the information in 
different places, then we're going to need some sort of accessing 
function like that
>
>> where it is indexing the table with the gimple_expr_code..
>> the stmt being processed was for a pointer assignment,
>>    _5 = _33
>> and it was coming back with a gimple_expr_code of  VAR_DECL instead of
>> an SSA_NAME... which confused me greatly.
>>
>>
>> gimple_expr_code (const gimple *stmt)
>> {
>>    enum gimple_code code = gimple_code (stmt);
>>    if (code == GIMPLE_ASSIGN || code == GIMPLE_COND)
>>      return (enum tree_code) stmt->subcode;
>>
>> A little more digging shows this:
>>
>> static inline enum tree_code
>> gimple_assign_rhs_code (const gassign *gs)
>> {
>>    enum tree_code code = (enum tree_code) gs->subcode;
>>    /* While we initially set subcode to the TREE_CODE of the rhs for
>>       GIMPLE_SINGLE_RHS assigns we do not update that subcode to stay
>>       in sync when we rewrite stmts into SSA form or do SSA
>> propagations.  */
>>    if (get_gimple_rhs_class (code) == GIMPLE_SINGLE_RHS)
>>      code = TREE_CODE (gs->op[1]);
>>
>>    return code;
>> }
>>
>> Fascinating comment.
> ... 😬
>
>> But it means that gimple_expr_code() isn't returning the correct result
>>
>> for GIMPLE_SINGLE_RHS....
> It depends. A SSA name isn't an expression code either. As said, the generic gimple_expr_code should be used with extreme care.

what is an expression code?  It seems like its just a  tree_code 
representing what is on the RHS?    Im not sure I understand why one 
needs to be careful with it.  It only applies to COND, ASSIGN and CALL. 
and its current right for everything except GIMPLE_SINGLE_RHS?

If we dont fix gimple_expr_code, then Im basically going to be 
reimplementing it myself... which seems kind of pointless.

Andrew
Richard Biener Nov. 13, 2020, 9:05 a.m. UTC | #3 
On Thu, Nov 12, 2020 at 10:12 PM Andrew MacLeod <amacleod@redhat.com> wrote:

> On 11/12/20 3:53 PM, Richard Biener wrote:
> > On November 12, 2020 9:43:52 PM GMT+01:00, Andrew MacLeod via
> Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> >> So I spent some time tracking down a ranger issue, and in the end, it
> >> boiled down to the range-op handler not being picked up properly.
> >>
> >> The handler is picked up by:
> >>
> >>    if ((gimple_code (s) == GIMPLE_ASSIGN) || (gimple_code (s) ==
> >> GIMPLE_COND))
> >>      return range_op_handler (gimple_expr_code (s), gimple_expr_type
> >> (s));
> > IMHO this should use more specific functions. Gimple_expr_code should go
> away similar to gimple_expr_type.
>
> gimple_expr_type is quite pervasive.. and each consumer is going to have
> to roll their own version of it.  Why do we want to get rid of it?
>
> If we are trying to save a few bytes by storing the information in
> different places, then we're going to need some sort of accessing
> function like that
> >
> >> where it is indexing the table with the gimple_expr_code..
> >> the stmt being processed was for a pointer assignment,
> >>    _5 = _33
> >> and it was coming back with a gimple_expr_code of  VAR_DECL instead of
> >> an SSA_NAME... which confused me greatly.
> >>
> >>
> >> gimple_expr_code (const gimple *stmt)
> >> {
> >>    enum gimple_code code = gimple_code (stmt);
> >>    if (code == GIMPLE_ASSIGN || code == GIMPLE_COND)
> >>      return (enum tree_code) stmt->subcode;
> >>
> >> A little more digging shows this:
> >>
> >> static inline enum tree_code
> >> gimple_assign_rhs_code (const gassign *gs)
> >> {
> >>    enum tree_code code = (enum tree_code) gs->subcode;
> >>    /* While we initially set subcode to the TREE_CODE of the rhs for
> >>       GIMPLE_SINGLE_RHS assigns we do not update that subcode to stay
> >>       in sync when we rewrite stmts into SSA form or do SSA
> >> propagations.  */
> >>    if (get_gimple_rhs_class (code) == GIMPLE_SINGLE_RHS)
> >>      code = TREE_CODE (gs->op[1]);
> >>
> >>    return code;
> >> }
> >>
> >> Fascinating comment.
> > ... 😬
> >
> >> But it means that gimple_expr_code() isn't returning the correct result
> >>
> >> for GIMPLE_SINGLE_RHS....
> > It depends. A SSA name isn't an expression code either. As said, the
> generic gimple_expr_code should be used with extreme care.
>
> what is an expression code?  It seems like its just a  tree_code
> representing what is on the RHS?    Im not sure I understand why one
> needs to be careful with it.  It only applies to COND, ASSIGN and CALL.
> and its current right for everything except GIMPLE_SINGLE_RHS?
>
> If we dont fix gimple_expr_code, then Im basically going to be
> reimplementing it myself... which seems kind of pointless.
>

Well sure we can fix it.  Your patch looks OK but can be optimized like

  if (gassign *ass = dyn_cast<gassign *> (stmt))
    return gimple_assign_rhs_code (stmt);

note it looks odd that we use this for gimple_assign but
directly access subcode for GIMPLE_COND instead
of returning gimple_cond_code () (again, operate on
gcond to avoid an extra check).

Thanks,
Richard.


> Andrew
>
>
>
>
diff mbox series

Patch

	* gimple.h (gimple_expr_code): Return gimple_assign_rhs_code
	for GIMPLE_ASSIGN.

diff --git a/gcc/gimple.h b/gcc/gimple.h
index 62b5a8a6124..8ef2f83d412 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -2229,26 +2229,6 @@  gimple_set_modified (gimple *s, bool modifiedp)
 }
 
 
-/* Return the tree code for the expression computed by STMT.  This is
-   only valid for GIMPLE_COND, GIMPLE_CALL and GIMPLE_ASSIGN.  For
-   GIMPLE_CALL, return CALL_EXPR as the expression code for
-   consistency.  This is useful when the caller needs to deal with the
-   three kinds of computation that GIMPLE supports.  */
-
-static inline enum tree_code
-gimple_expr_code (const gimple *stmt)
-{
-  enum gimple_code code = gimple_code (stmt);
-  if (code == GIMPLE_ASSIGN || code == GIMPLE_COND)
-    return (enum tree_code) stmt->subcode;
-  else
-    {
-      gcc_gimple_checking_assert (code == GIMPLE_CALL);
-      return CALL_EXPR;
-    }
-}
-
-
 /* Return true if statement STMT contains volatile operands.  */
 
 static inline bool
@@ -2889,6 +2869,29 @@  gimple_assign_cast_p (const gimple *s)
   return false;
 }
 
+
+/* Return the tree code for the expression computed by STMT.  This is
+   only valid for GIMPLE_COND, GIMPLE_CALL and GIMPLE_ASSIGN.  For
+   GIMPLE_CALL, return CALL_EXPR as the expression code for
+   consistency.  This is useful when the caller needs to deal with the
+   three kinds of computation that GIMPLE supports.  */
+
+static inline enum tree_code
+gimple_expr_code (const gimple *stmt)
+{
+  enum gimple_code code = gimple_code (stmt);
+  if (code == GIMPLE_ASSIGN)
+    return gimple_assign_rhs_code (stmt);
+  else if (code == GIMPLE_COND)
+    return (enum tree_code) stmt->subcode;
+  else
+    {
+      gcc_gimple_checking_assert (code == GIMPLE_CALL);
+      return CALL_EXPR;
+    }
+}
+
+
 /* Return true if S is a clobber statement.  */
 
 static inline bool