This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH GCC][1/2]Feed bound computation to folder in loop split
Marc Glisse <marc.glisse@inria.fr> writes:
> On Wed, 26 Jul 2017, Richard Sandiford wrote:
>> Richard Biener <richard.guenther@gmail.com> writes:
>>> On Tue, Jul 25, 2017 at 7:45 PM, Marc Glisse <marc.glisse@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 <rguenther@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 <rguenther@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