diff mbox series

Re: Fix gimple_expr_code?

Message ID da8c43fe-5791-8f7d-fd7b-0b72431f9593@redhat.com
State New
Headers show
Series Re: Fix gimple_expr_code? | expand

Commit Message

Andrew MacLeod Nov. 13, 2020, 7 p.m. UTC 
On 11/13/20 4:05 AM, Richard Biener wrote:
> On Thu, Nov 12, 2020 at 10:12 PM Andrew MacLeod <amacleod@redhat.com 
> <mailto: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
>     <mailto: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
>
>
>
And with a little bit of const-ness...  I adjusted gimple_range_handler 
to use gassing and gcond as well.

Bootstrapped on x86_64-pc-linux-gnu, no regressions. pushed.

Andrew
diff mbox series

Patch

commit fcbb6018abaf04d30e2cf6fff2eb35115412cdd5
Author: Andrew MacLeod <amacleod@redhat.com>
Date:   Fri Nov 13 13:56:01 2020 -0500

    Re: Fix gimple_expr_code?
    
    have gimple_expr_code return the correct code for GIMPLE_ASSIGN.
    use gassign and gcond in gimple_range_handler.
    
            * gimple-range.h (gimple_range_handler): Cast to gimple stmt
            kinds before asking for code and type.
            * gimple.h (gimple_expr_code): Call gassign and gcond routines
            to get their expr_code.

diff --git a/gcc/gimple-range.h b/gcc/gimple-range.h
index dde41e9e743..92bb5305c18 100644
--- a/gcc/gimple-range.h
+++ b/gcc/gimple-range.h
@@ -97,12 +97,12 @@  extern bool gimple_range_calc_op2 (irange &r, const gimple *s,
 static inline range_operator *
 gimple_range_handler (const gimple *s)
 {
-  if (gimple_code (s) == GIMPLE_ASSIGN)
-    return range_op_handler (gimple_assign_rhs_code (s),
-			     TREE_TYPE (gimple_assign_lhs (s)));
-  if (gimple_code (s) == GIMPLE_COND)
-    return range_op_handler (gimple_cond_code (s),
-			     TREE_TYPE (gimple_cond_lhs (s)));
+  if (const gassign *ass = dyn_cast<const gassign *> (s))
+    return range_op_handler (gimple_assign_rhs_code (ass),
+			     TREE_TYPE (gimple_assign_lhs (ass)));
+  if (const gcond *cond = dyn_cast<const gcond *> (s))
+    return range_op_handler (gimple_cond_code (cond),
+			     TREE_TYPE (gimple_cond_lhs (cond)));
   return NULL;
 }
 
diff --git a/gcc/gimple.h b/gcc/gimple.h
index d359f7827b1..b935ad4f761 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
@@ -3793,6 +3773,28 @@  gimple_cond_set_condition (gcond *stmt, enum tree_code code, tree lhs,
   gimple_cond_set_rhs (stmt, rhs);
 }
 
+
+/* 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)
+{
+  if (const gassign *ass = dyn_cast<const gassign *> (stmt))
+    return gimple_assign_rhs_code (ass);
+  if (const gcond *cond = dyn_cast<const gcond *> (stmt))
+    return gimple_cond_code (cond);
+  else
+    {
+      gcc_gimple_checking_assert (gimple_code (stmt) == GIMPLE_CALL);
+      return CALL_EXPR;
+    }
+}
+
+
 /* Return the LABEL_DECL node used by GIMPLE_LABEL statement GS.  */
 
 static inline tree