Allow more complex call replacements in gimple-fold.c

An upcoming patch adds a match.pd rule that folds pow(pow(x,y),z)
to pow(x,y*z).  This fold can reuse the existing pow gimple statement
and simply replace the operands with x and y*z.  However, the y*z
itself requires a separate gimple statement and the code wasn't
prepared to handle that.

Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabi.
OK to install?

Thanks,
Richard


gcc/
	* gimple-fold.c (replace_stmt_with_simplification): Allow calls
	to have nonempty sequences.

On Mon, Oct 26, 2015 at 10:41 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> An upcoming patch adds a match.pd rule that folds pow(pow(x,y),z)
> to pow(x,y*z).  This fold can reuse the existing pow gimple statement
> and simply replace the operands with x and y*z.  However, the y*z
> itself requires a separate gimple statement and the code wasn't
> prepared to handle that.
>
> Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabi.
> OK to install?

Hmm, I put the assert there because of the !inplace case but I see the
is_tree_code case alredy behaves the same.

I think the intent of fold_stmt_inplace (and thus the 'inplace' case)
was that no additional stmts are inserted (and obviously the
stmt itself being not replaced, so gsi_stmt () is the same before and
after).

So I think both the is_gimple_assign && is_tree_code and the
case you are amending need a

  if (inplace && !gimple_seq_empty_p (*seq))
   return false;

Thanks,
Richard.

> Thanks,
> Richard
>
>
> gcc/
>         * gimple-fold.c (replace_stmt_with_simplification): Allow calls
>         to have nonempty sequences.
>
> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
> index 1869c09..4b9b782 100644
> --- a/gcc/gimple-fold.c
> +++ b/gcc/gimple-fold.c
> @@ -3365,7 +3365,14 @@ replace_stmt_with_simplification (gimple_stmt_iterator *gsi,
>         }
>        if (i < 3)
>         gcc_assert (ops[i] == NULL_TREE);
> -      gcc_assert (gimple_seq_empty_p (*seq));
> +      if (dump_file && (dump_flags & TDF_DETAILS))
> +       {
> +         fprintf (dump_file, "gimple_simplified to ");
> +         if (!gimple_seq_empty_p (*seq))
> +           print_gimple_seq (dump_file, *seq, 0, TDF_SLIM);
> +         print_gimple_stmt (dump_file, gsi_stmt (*gsi), 0, TDF_SLIM);
> +       }
> +      gsi_insert_seq_before (gsi, *seq, GSI_SAME_STMT);
>        return true;
>      }
>    else if (!inplace)
>

Richard Biener <richard.guenther@gmail.com> writes:
> On Mon, Oct 26, 2015 at 10:41 AM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> An upcoming patch adds a match.pd rule that folds pow(pow(x,y),z)
>> to pow(x,y*z).  This fold can reuse the existing pow gimple statement
>> and simply replace the operands with x and y*z.  However, the y*z
>> itself requires a separate gimple statement and the code wasn't
>> prepared to handle that.
>>
>> Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabi.
>> OK to install?
>
> Hmm, I put the assert there because of the !inplace case but I see the
> is_tree_code case alredy behaves the same.
>
> I think the intent of fold_stmt_inplace (and thus the 'inplace' case)
> was that no additional stmts are inserted (and obviously the
> stmt itself being not replaced, so gsi_stmt () is the same before and
> after).
>
> So I think both the is_gimple_assign && is_tree_code and the
> case you are amending need a
>
>   if (inplace && !gimple_seq_empty_p (*seq))
>    return false;

OK.  I was wondering about that, but the comment isn't clear whether
inserting extra statements before the original statement is OK, as long
as the final statement has the same form.

If this hasn't caused problems for tree codes, do you think that means
that callers don't care about extra statements matter in practice,
that there simply aren't many fold patterns like this, or that most
cases where this kind of fold hits are caught earlier?  Just worried
about introducing a pessimisation...

Should we simply have that check before:

  if (gcond *cond_stmt = dyn_cast <gcond *> (stmt))

so that it's common to all cases?

Thanks,
Richard

On Mon, Oct 26, 2015 at 11:30 AM, Richard Sandiford
<richard.sandiford@arm.com> wrote:
> Richard Biener <richard.guenther@gmail.com> writes:
>> On Mon, Oct 26, 2015 at 10:41 AM, Richard Sandiford
>> <rdsandiford@googlemail.com> wrote:
>>> An upcoming patch adds a match.pd rule that folds pow(pow(x,y),z)
>>> to pow(x,y*z).  This fold can reuse the existing pow gimple statement
>>> and simply replace the operands with x and y*z.  However, the y*z
>>> itself requires a separate gimple statement and the code wasn't
>>> prepared to handle that.
>>>
>>> Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabi.
>>> OK to install?
>>
>> Hmm, I put the assert there because of the !inplace case but I see the
>> is_tree_code case alredy behaves the same.
>>
>> I think the intent of fold_stmt_inplace (and thus the 'inplace' case)
>> was that no additional stmts are inserted (and obviously the
>> stmt itself being not replaced, so gsi_stmt () is the same before and
>> after).
>>
>> So I think both the is_gimple_assign && is_tree_code and the
>> case you are amending need a
>>
>>   if (inplace && !gimple_seq_empty_p (*seq))
>>    return false;
>
> OK.  I was wondering about that, but the comment isn't clear whether
> inserting extra statements before the original statement is OK, as long
> as the final statement has the same form.
>
> If this hasn't caused problems for tree codes, do you think that means
> that callers don't care about extra statements matter in practice,
> that there simply aren't many fold patterns like this, or that most
> cases where this kind of fold hits are caught earlier?  Just worried
> about introducing a pessimisation...

There are not many passes restricting folding to inplace anymore.  Maybe
it's again time to re-evaluate some of them.

>
> Should we simply have that check before:
>
>   if (gcond *cond_stmt = dyn_cast <gcond *> (stmt))
>
> so that it's common to all cases?

Yeah, that makes sense.

Thanks,
Richard.

> Thanks,
> Richard
>