Re: [PATCH, PR43236] Miscompilations with tree-loop-distribution

[Richard Guenther <richard dot guenther at gmail dot com>]

On Wed, Mar 10, 2010 at 12:10 PM, Alexander Monakov <amonakov@ispras.ru> wrote:
> This patch fixes two wrong-code bugs in tree loop distribution, both of which
> are demonstrated with the same testcase at different optimization levels (-Os,
> -O[123]).
>
> The first problem is wrong calculation of base address for memset zero: it is
> (first_access - niter), while it should be (first_access - niter + 1).
> Note that generate_memset_zero may benefit from slight refactoring of memset
> args construction: if/else blocks are very similar. ?I can do that as a
> followup.
>
> The second problem is that assignment may end up in the latch block, in which
> case we should use number_of_latch_executions, not number_of_exit_cond_execs.
> This happens with -Os on the testcase.
>
> Fixed as follows, bootstrapped and regtested with loop distribution enabled by
> default on x86_64-linux. ?Ok for 4.6? ?Ok for trunk (the changes are isolated
> and the previous loop distribution patch was approved for trunk)?

This is ok for trunk.

Thanks,
Richard.

> 2010-08-10 ?Alexander Monakov ?<amonakov@ispras.ru>
>
> ? ? ? ?PR tree-optimization/43236
> ? ? ? ?* tree-loop-distribution.c (generate_memset_zero): Fix off-by-one
> ? ? ? ?error is calculation of base access in reverse iteration case.
> ? ? ? ?(generate_builtin): Take number of latch executions if the statement
> ? ? ? ?is in the latch.
>
> ? ? ? ?* gcc.c-torture/execute/pr43236.c: New.
>
> diff --git a/gcc/tree-loop-distribution.c b/gcc/tree-loop-distribution.c
> index 920ab8c..74120c6 100644
> --- a/gcc/tree-loop-distribution.c
> +++ b/gcc/tree-loop-distribution.c
> @@ -285,6 +285,8 @@ generate_memset_zero (gimple stmt, tree op0, tree nb_iter,
> ? ? ? addr_base = fold_convert_loc (loc, sizetype, addr_base);
> ? ? ? addr_base = size_binop_loc (loc, MINUS_EXPR, addr_base,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?fold_convert_loc (loc, sizetype, nb_bytes));
> + ? ? ?addr_base = size_binop_loc (loc, PLUS_EXPR, addr_base,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? TYPE_SIZE_UNIT (TREE_TYPE (op0)));
> ? ? ? addr_base = fold_build2_loc (loc, POINTER_PLUS_EXPR,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? TREE_TYPE (DR_BASE_ADDRESS (dr)),
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DR_BASE_ADDRESS (dr), addr_base);
> @@ -389,6 +391,8 @@ generate_builtin (struct loop *loop, bitmap partition, bool copy_p)
> ? ? ? ? ? ? ? ?goto end;
>
> ? ? ? ? ? ? ?write = stmt;
> + ? ? ? ? ? ? if (bb == loop->latch)
> + ? ? ? ? ? ? ? nb_iter = number_of_latch_executions (loop);
> ? ? ? ? ? ?}
> ? ? ? ?}
> ? ? }
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr43236.c b/gcc/testsuite/gcc.c-torture/execute/pr43236.c
> new file mode 100644
> index 0000000..0401c88
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr43236.c
> @@ -0,0 +1,32 @@
> +/* { dg-options "-ftree-loop-distribution" } */
> +extern void abort(void);
> +extern void *memset(void *s, int c, __SIZE_TYPE__ n);
> +extern int memcmp(const void *s1, const void *s2, __SIZE_TYPE__ n);
> +/*extern int printf(const char *format, ...);*/
> +
> +int main()
> +{
> + ?char A[30], B[30], C[30];
> + ?int i;
> +
> + ?/* prepare arrays */
> + ?memset(A, 1, 30);
> + ?memset(B, 1, 30);
> +
> + ?for (i = 20; i-- > 10;) {
> + ? ?A[i] = 0;
> + ? ?B[i] = 0;
> + ?}
> +
> + ?/* expected result */
> + ?memset(C, 1, 30);
> + ?memset(C + 10, 0, 10);
> +
> + ?/* show result */
> +/* ?for (i = 0; i < 30; i++)
> + ? ?printf("%d %d %d\n", A[i], B[i], C[i]); */
> +
> + ?/* compare results */
> + ?if (memcmp(A, C, 30) || memcmp(B, C, 30)) abort();
> + ?return 0;
> +}
>
>