On Thu, Nov 12, 2020 at 10:12 PM Andrew MacLeod <amacl...@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
>
>
>
>
