This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] -ftree-loop-linear fixes (PR tree-optimization/46970) (take 2)


On Mon, Dec 20, 2010 at 11:28 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Sun, Dec 19, 2010 at 11:05:07PM -0600, Sebastian Pop wrote:
>> The patch looks good to me, but I cannot approve it.
>>
>> > ? /* We might have some leftover. ?*/
>> > - ?if (gimple_cond_code (exit_cond) == LT_EXPR)
>> > - ? ?extra = -1 * stepint;
>> > - ?else if (gimple_cond_code (exit_cond) == NE_EXPR)
>> > - ? ?extra = -1 * stepint;
>> > - ?else if (gimple_cond_code (exit_cond) == GT_EXPR)
>> > - ? ?extra = -1 * stepint;
>> > - ?else if (gimple_cond_code (exit_cond) == EQ_EXPR)
>> > - ? ?extra = 1 * stepint;
>> > + ?if (SSA_NAME_DEF_STMT (inductionvar) != phi)
>> > + ? ?{
>> > + ? ? ?if (gimple_cond_code (exit_cond) == LT_EXPR)
>> > + ? ? ? extra = -1 * stepint;
>> > + ? ? ?else if (gimple_cond_code (exit_cond) == NE_EXPR)
>> > + ? ? ? extra = -1 * stepint;
>> > + ? ? ?else if (gimple_cond_code (exit_cond) == GT_EXPR)
>> > + ? ? ? extra = -1 * stepint;
>> > + ? ? ?else if (gimple_cond_code (exit_cond) == EQ_EXPR)
>> > + ? ? ? extra = 1 * stepint;
>> > + ? ?}
>> > + ?else
>> > + ? ?{
>> > + ? ? ?if (gimple_cond_code (exit_cond) == LE_EXPR)
>> > + ? ? ? extra = 1 * stepint;
>> > + ? ? ?else if (gimple_cond_code (exit_cond) == GE_EXPR)
>> > + ? ? ? extra = 1 * stepint;
>> > + ? ?}
>>
>> It would be easier to read the following equivalent code:
>
> Or a switch, as done in this patch, bootstrapped/regtested on x86_64-linux
> and i686-linux:
>
> 2010-12-20 ?Jakub Jelinek ?<jakub@redhat.com>
>
> ? ? ? ?PR tree-optimization/46970
> ? ? ? ?* lambda-code.c (gcc_loop_to_lambda_loop): Give up if
> ? ? ? ?step doesn't fit into host int or if def stmt of inductionvar
> ? ? ? ?is neither PHI nor increment by step. ?If exit condition
> ? ? ? ?compares induction variable before increment, adjust ubound
> ? ? ? ?differently.
>
> ? ? ? ?* gcc.dg/pr46970-1.c: New test.
> ? ? ? ?* gcc.dg/pr46970-2.c: New test.
>
> --- gcc/lambda-code.c.jj ? ? ? ?2010-08-20 16:05:41.000000000 +0200
> +++ gcc/lambda-code.c ? 2010-12-20 21:19:12.000000000 +0100
> @@ -1289,7 +1289,7 @@ gcc_loop_to_lambda_loop (struct loop *lo
> ? if (gimple_code (phi) != GIMPLE_PHI)
> ? ? {
> ? ? ? tree op = SINGLE_SSA_TREE_OPERAND (phi, SSA_OP_USE);
> - ? ? ?if (!op)
> + ? ? ?if (!op || !is_gimple_assign (phi))
> ? ? ? ?{
>
> ? ? ? ? ?if (dump_file && (dump_flags & TDF_DETAILS))
> @@ -1332,7 +1332,9 @@ gcc_loop_to_lambda_loop (struct loop *lo
>
> ? ? ? return NULL;
> ? ? }
> - ?if (TREE_CODE (step) != INTEGER_CST)
> + ?if (!host_integerp (step, 0)
> + ? ? ?|| (HOST_WIDE_INT) TREE_INT_CST_LOW (step)
> + ? ? ? ? != (int) TREE_INT_CST_LOW (step))

That casting looks odd.  Why isn't the (supposed) use of step as 'int' not
changed to a HOST_WIDE_INT?

> ? ? {
>
> ? ? ? if (dump_file && (dump_flags & TDF_DETAILS))
> @@ -1366,6 +1368,32 @@ gcc_loop_to_lambda_loop (struct loop *lo
> ? ? ? return NULL;
> ? ? }
>
> + ?if (SSA_NAME_DEF_STMT (inductionvar) != phi)
> + ? ?{
> + ? ? ?gimple stmt = SSA_NAME_DEF_STMT (inductionvar);
> + ? ? ?tree rhs2;
> + ? ? ?int stepintval = stepint;
> + ? ? ?switch (gimple_assign_rhs_code (stmt))
> + ? ? ? {
> + ? ? ? case MINUS_EXPR:
> + ? ? ? ? stepintval = -stepint;
> + ? ? ? ? /* FALLTHRU */
> + ? ? ? case PLUS_EXPR:
> + ? ? ? case POINTER_PLUS_EXPR:
> + ? ? ? ? rhs2 = gimple_assign_rhs2 (stmt);
> + ? ? ? ? if (TREE_CODE (rhs2) == INTEGER_CST
> + ? ? ? ? ? ? && (HOST_WIDE_INT) TREE_INT_CST_LOW (rhs2) == stepintval
> + ? ? ? ? ? ? && TREE_INT_CST_HIGH (rhs2) == (stepintval >= 0 ? 0 : -1))

That would also simplify this code - why not tree_int_cst_equal (step,
rhs2) btw?

The rest of the patch looks sensible, though I know nothing about
tree-loop-linear.

Richard.

> + ? ? ? ? ? break;
> + ? ? ? ? /* FALLTHRU */
> + ? ? ? default:
> + ? ? ? ? if (dump_file && (dump_flags & TDF_DETAILS))
> + ? ? ? ? ? fprintf (dump_file,
> + ? ? ? ? ? ? ? ? ? ?"Unable to convert loop: Cannot find PHI node for induction variable\n");
> + ? ? ? ? return NULL;
> + ? ? ? }
> + ? ?}
> +
> ? if (flow_bb_inside_loop_p (loop, gimple_phi_arg_edge (phi, 0)->src))
> ? ? {
> ? ? ? lboundvar = PHI_ARG_DEF (phi, 1);
> @@ -1417,14 +1445,30 @@ gcc_loop_to_lambda_loop (struct loop *lo
>
>
> ? /* We might have some leftover. ?*/
> - ?if (gimple_cond_code (exit_cond) == LT_EXPR)
> - ? ?extra = -1 * stepint;
> - ?else if (gimple_cond_code (exit_cond) == NE_EXPR)
> - ? ?extra = -1 * stepint;
> - ?else if (gimple_cond_code (exit_cond) == GT_EXPR)
> - ? ?extra = -1 * stepint;
> - ?else if (gimple_cond_code (exit_cond) == EQ_EXPR)
> - ? ?extra = 1 * stepint;
> + ?if (SSA_NAME_DEF_STMT (inductionvar) != phi)
> + ? ?switch (gimple_cond_code (exit_cond))
> + ? ? ?{
> + ? ? ?case LT_EXPR:
> + ? ? ?case NE_EXPR:
> + ? ? ?case GT_EXPR:
> + ? ? ? extra = -1 * stepint;
> + ? ? ? break;
> + ? ? ?case EQ_EXPR:
> + ? ? ? extra = 1 * stepint;
> + ? ? ? break;
> + ? ? ?default:
> + ? ? ? break;
> + ? ? ?}
> + ?else
> + ? ?switch (gimple_cond_code (exit_cond))
> + ? ? ?{
> + ? ? ?case LE_EXPR:
> + ? ? ?case GE_EXPR:
> + ? ? ? extra = 1 * stepint;
> + ? ? ? break;
> + ? ? ?default:
> + ? ? ? break;
> + ? ? ?}
>
> ? ubound = gcc_tree_to_linear_expression (depth, uboundvar,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?outerinductionvars,
> --- gcc/testsuite/gcc.dg/pr46970-1.c.jj 2010-12-17 16:08:01.000000000 +0100
> +++ gcc/testsuite/gcc.dg/pr46970-1.c ? ?2010-12-17 16:07:46.000000000 +0100
> @@ -0,0 +1,27 @@
> +/* PR tree-optimization/46970 */
> +/* { dg-do run } */
> +/* { dg-options "-Os -ftree-loop-linear" } */
> +
> +int
> +foo (int n, int *a)
> +{
> + ?int i, j;
> +
> + ?for (i = 0; i < n; i++)
> + ? ?for (j = 0; j < n; j++)
> + ? ? ?a[j] = i + n;
> +
> + ?for (j = 0; j < n; j++)
> + ? ?if (a[j] != i + n - 1)
> + ? ? ?__builtin_abort ();
> +
> + ?return 0;
> +}
> +
> +int
> +main ()
> +{
> + ?int a[16];
> + ?foo (16, a);
> + ?return 0;
> +}
> --- gcc/testsuite/gcc.dg/pr46970-2.c.jj 2010-12-17 18:01:05.000000000 +0100
> +++ gcc/testsuite/gcc.dg/pr46970-2.c ? ?2010-12-17 16:43:46.000000000 +0100
> @@ -0,0 +1,28 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -ftree-loop-linear" } */
> +/* { dg-require-effective-target size32plus } */
> +
> +double u[1782225];
> +
> +__attribute__((noinline, noclone)) int
> +foo (int N, int *res)
> +{
> + ?unsigned int i, j;
> + ?double sum = 0;
> + ?for (i = 0; i < N; i++)
> + ? ?for (j = 0; j < N; j++)
> + ? ? ?sum = sum + u[i + 1335 * j];
> + ?*res = sum + N;
> +}
> +
> +int
> +main (void)
> +{
> + ?int i;
> + ?for (i = 0; i < 1782225; i++)
> + ? ?u[i] = 1.0;
> + ?foo (64, &i);
> + ?if (i != 4160)
> + ? ?__builtin_abort ();
> + ?return 0;
> +}
>
> ? ? ? ?Jakub
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]