Marc Glisse <marc.gli...@inria.fr> writes: > On Wed, 26 Jul 2017, Richard Sandiford wrote: >> Richard Biener <richard.guent...@gmail.com> writes: >>> On Tue, Jul 25, 2017 at 7:45 PM, Marc Glisse <marc.gli...@inria.fr> wrote: >>>> On Tue, 25 Jul 2017, Richard Biener wrote: >>>> >>>>>> I think we need Richard to say what the intent is for the valueization >>>>>> function. It is used both to stop looking at defining stmt if the return >>>>>> is >>>>>> NULL, and to replace/optimize one SSA_NAME with another, but currently it >>>>>> seems hard to prevent looking at the defining statement without >>>>>> preventing >>>>>> from looking at the SSA_NAME at all. >>>>> >>>>> >>>>> Yeah, this semantic overloading is an issue. For gimple_build we have >>>>> nothing >>>>> to "valueize" but we only use it to tell genmatch that it may not look at >>>>> the >>>>> SSA_NAME_DEF_STMT. >>>>> >>>>>> I guess we'll need a fix in genmatch... >>>>> >>>>> >>>>> I'll have a look tomorrow. >>>> >>>> >>>> My impression yesterday was that we could replace the current do_valueize >>>> wrapper by 2 wrappers (without touching the valueize callbacks): >>>> - may_check_def_stmt, which returns a bool corresponding to the current >>>> do_valueize != NULL_TREE >>>> - maybe_valueize, which tries to valueize, but if it gets a NULL_TREE, it >>>> returns its argument unchanged. >>>> >>>> Not very confident about it though. >>> >>> Note I've been there in the past (twice I think) but always ran into >>> existing >>> latent issues. So hopefully we've resolved those, I'm testing the following >>> simplified variant of what I had back in time. >>> >>> It'll produce >>> >>> switch (TREE_CODE (op0)) >>> { >>> case SSA_NAME: >>> if (gimple *def_stmt = get_def (valueize, op0)) >>> { >>> if (gassign *def = dyn_cast <gassign *> (def_stmt)) >>> switch (gimple_assign_rhs_code (def)) >>> { >>> case MINUS_EXPR: >>> { >>> tree o20 = gimple_assign_rhs1 (def); >>> o20 = do_valueize (valueize, o20); >>> tree o21 = gimple_assign_rhs2 (def); >>> o21 = do_valueize (valueize, o21); >>> if (op1 == o21 || (operand_equal_p (op1, o21, 0) && >>> types_match (op1, o21))) >>> { >>> >>> which also indents less which is nice. >>> >>> Bootstrap/regtest running on x86_64-unknown-linux-gnu. >>> >>> Richard. >>> >>> 2017-07-26 Richard Biener <rguent...@suse.de> >>> >>> * gimple-match-head.c (do_valueize): Return OP if valueize >>> returns NULL_TREE. >>> (get_def): New helper to get at the def stmt of a SSA name >>> if valueize allows. >>> * genmatch.c (dt_node::gen_kids_1): Use get_def instead of >>> do_valueize to get at the def stmt. >>> (dt_operand::gen_gimple_expr): Simplify do_valueize calls. >>> >>> >>>> -- >>>> Marc Glisse >>> >>> 2017-07-26 Richard Biener <rguent...@suse.de> >>> >>> * gimple-match-head.c (do_valueize): Return OP if valueize >>> returns NULL_TREE. >>> (get_def): New helper to get at the def stmt of a SSA name >>> if valueize allows. >>> * genmatch.c (dt_node::gen_kids_1): Use get_def instead of >>> do_valueize to get at the def stmt. >>> (dt_operand::gen_gimple_expr): Simplify do_valueize calls. >>> >>> Index: gcc/gimple-match-head.c >>> =================================================================== >>> --- gcc/gimple-match-head.c (revision 250518) >>> +++ gcc/gimple-match-head.c (working copy) >>> @@ -779,10 +779,25 @@ inline tree >>> do_valueize (tree (*valueize)(tree), tree op) >>> { >>> if (valueize && TREE_CODE (op) == SSA_NAME) >>> - return valueize (op); >>> + { >>> + tree tem = valueize (op); >>> + if (tem) >>> + return tem; >>> + } >>> return op; >>> } >>> >>> +/* Helper for the autogenerated code, get at the definition of NAME when >>> + VALUEIZE allows that. */ >>> + >>> +inline gimple * >>> +get_def (tree (*valueize)(tree), tree name) >>> +{ >>> + if (valueize && ! valueize (name)) >>> + return NULL; >>> + return SSA_NAME_DEF_STMT (name); >>> +} >> >> I realise this is preexisting, but why do we ignore the value returned >> by valueize, even if it's different from NAME? > > My impression is that we expect it has already been valueized.
But in that case, why do we try to valueize it again? It feels like we're conflating two things. Richard