Re: [PATCH] Fix up adjust_return_value_with_ops (PR tree-optimization/56539)

[Jeff Law <law@redhat.com>]

On 03/06/2013 09:43 AM, Jakub Jelinek wrote:
> Hi!
>
> This patch fixes a bug where force_gimple_operand_gsi was called with
> bogus last argument.  The before argument is true, and the stmt that is
> added at the end of course should be put in between the (optional) sequence
> added by force_gimple_operand_gsi and gsi_stmt (gsi), so it should use
> GSI_SAME_STMT, otherwise we end up using SSA_NAME in stmt defined later on
> within the same bb.
>
> Bootstrapped on i686-linux, bootstrap on x86_64-linux and both regtests
> still pending, ok for trunk if it succeeds?
>
> BTW, I think we still have a latent issue there, because in the testcase
> the computations are done in unsigned short type (both multiplications
> and additions), so they never cause undefined behavior on overflow, but
> in what tail recursion generates, the updates on a_acc and m_acc look fine
> (done in unsigned short type), the accumulators themselves are signed short
> (in theory fine too), but at the end the additions and multiplications
> with a_acc/m_acc are done in signed short type.  I guess I should open a PR
> for that.  E.g. for the simpler:
> short foo (const char *x, unsigned y) { return y > 1 ? 10 * foo (x, y - 1) : (*x - '0'); }
> there is signed short multiplication at the end rather than unsigned already
> in GCC 4.4.
>
> 2013-03-06  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR tree-optimization/56539
> 	* tree-tailcall.c (adjust_return_value_with_ops): Use GSI_SAME_STMT
> 	instead of GSI_CONTINUE_LINKING as last argument to
> 	force_gimple_operand_gsi.
>
> 	* gcc.c-torture/compile/pr56539.c: New test.
Doesn't the code in update_accumulator_with_ops need the same change? 
Unrelated, but the block comment still refers to UPDATE, which is no 
longer a parameter.

I see similar looking code in tree-inline.c::copy_bb...  Does it need 
updating as well?


Change looks OK to me, but please check those other two instances and 
take appropriate action.  If you could update the block comment before 
adjust_return_value_with_ops it'd be appreciated as well.

jeff