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
On Wed, Jul 26, 2017 at 11:57 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Wed, 26 Jul 2017, Richard Sandiford wrote:
>
>> 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?
>
>
> We don't know if valueize returned NULL and we kept the argument as is
> (should not look at def stmt) or if valueize actually returned something.
> This way we can also have valueize replace a value with another, and still
> forbid looking at the def stmt of the replacement value.
>
>> It feels like we're conflating two things.
Yes we are...
> We are. The question is how much trouble that causes, compared to the
> complication of for instance having 2 callbacks.
Or doing (have a patch for that) magics on the return value (smuggling
in a bit on whether we may look at the def).
Doing that and adjusting the few callers we have that a) valueize
and b) return NULL sometimes ran into some bugs so I never
finished this off. The current patch is fixing a related issue, so if it
goes through (testing still runs) I can maybe dust-off that "proper" fix...
Richard.
> --
> Marc Glisse