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

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

[Jakub Jelinek]

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.

--- gcc/tree-tailcall.c.jj	2013-02-08 13:16:55.000000000 +0100
+++ gcc/tree-tailcall.c	2013-03-06 15:02:35.836468072 +0100
@@ -622,7 +622,7 @@ adjust_return_value_with_ops (enum tree_
 					    fold_convert (TREE_TYPE (op1), acc),
 					    op1));
       rhs = force_gimple_operand_gsi (&gsi, rhs,
-				      false, NULL, true, GSI_CONTINUE_LINKING);
+				      false, NULL, true, GSI_SAME_STMT);
       stmt = gimple_build_assign (result, rhs);
     }
 
--- gcc/testsuite/gcc.c-torture/compile/pr56539.c.jj	2013-03-06 15:05:50.696330194 +0100
+++ gcc/testsuite/gcc.c-torture/compile/pr56539.c	2013-03-06 15:05:32.000000000 +0100
@@ -0,0 +1,7 @@
+/* PR tree-optimization/56539 */
+
+short
+foo (const char *x, unsigned y)
+{
+  return y > 1 ? (x[y - 1] - '0') + 10 * foo (x, y - 1) : (*x - '0');
+}

	Jakub

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

[Jeff Law]

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

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

[Jakub Jelinek]

On Wed, Mar 06, 2013 at 03:00:49PM -0700, Jeff Law wrote:
> Doesn't the code in update_accumulator_with_ops need the same
> change?

No, the difference is that it uses false as the next to last argument,
i.e. inserts after gsi, in which case GSI_CONTINUE_LINKING is desirable,
so that the stmt is inserted after that.

> Unrelated, but the block comment still refers to UPDATE,
> which is no longer a parameter.

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

That is again false, GSI_CONTINUE_LINKING pair, i.e. insert after and update
gsi to point after the added stmts.

	Jakub

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

[Jeff Law]

On 03/06/2013 04:28 PM, Jakub Jelinek wrote:
> On Wed, Mar 06, 2013 at 03:00:49PM -0700, Jeff Law wrote:
>> Doesn't the code in update_accumulator_with_ops need the same
>> change?
>
> No, the difference is that it uses false as the next to last argument,
> i.e. inserts after gsi, in which case GSI_CONTINUE_LINKING is desirable,
> so that the stmt is inserted after that.
Thanks.  Just wanted to make sure -- I've never had to ponder how the 
GSI/BSI linking code works much.

Jeff