Fix gimple_expr_code?

[Richard Biener]

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
>
>
>
>