Re: [PATCH GCC][1/2]Feed bound computation to folder in loop split

[Marc Glisse <marc.glisse@inria.fr>]

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.

-- 
Marc Glisse