Re: [PATCH] Fix PR69726

[Christophe Lyon <christophe dot lyon at linaro dot org>]

On 9 February 2016 at 13:42, Richard Biener <rguenther@suse.de> wrote:
>
> It turns out if-conversions poor job on
>
>  if (a)
>    x[i] = ...;
>  else
>    x[i] = ...;
>
> results in bogus uninit warnings of x[i] for a variety of reasons.
> First of all forwprop (aka match.pd simplification) doesn't fixup
> all of if-conversions poor job as canonicalization sometimes
> inverts the condition in [VEC_]COND_EXPRs and thus the existing
> A ? B : (A ? X : C) -> A ? B : C pattern doesn't apply.  The match.pd
> hunk fixes this (albeit in an awkward way - I don't feel like mucking
> with genmatch at this stage, nor exactly for the poor [VEC_]COND_EXPR
> IL we should rather fix).  Second, the late uninit pass is confused
> by the left-over dead code, in this case dead load feeding a dead
> VEC_COND_EXPR.  Adding a DCE pass before late uninit as the comment
> in passes.def suggests fixes this and also should avoid creating the dead
> RTL I've sometimes seen.
>
> Due to the PR69719 fix we're now over the alias-test limit for the
> testcase (well, all alias tests are bogus, see PR69732), so I upped
> that limit for the testcase.  I'm investigating the Job done there.
>
> Bootstrap and regtest is currently running on x86_64-unknown-linux-gnu.
>
Hi,

After this patch, I noticed that
  g++.dg/tree-ssa/pr61034.C  -std=gnu++11  scan-tree-dump-times
optimized "free" 4
now fails on arm* and aarch64*
The dump contains no occurrence of "free".

Is this just a matter of adjusting the test-case or an unexpected
problem with this patch?

Christophe.

> Richard.
>
> 2016-02-09  Richard Biener  <rguenther@suse.de>
>
>         PR tree-optimization/69726
>         * passes.def: Add DCE pass before late uninit.
>         * match.pd: Add A ? B : (!A ? C : X) -> A ? B : C patterns to
>         really fixup if-conversions job.
>
>         * gcc.dg/uninit-22.c: New testcase.
>
> Index: gcc/passes.def
> ===================================================================
> *** gcc/passes.def      (revision 233241)
> --- gcc/passes.def      (working copy)
> *************** along with GCC; see the file COPYING3.
> *** 322,336 ****
>         NEXT_PASS (pass_fold_builtins);
>         NEXT_PASS (pass_optimize_widening_mul);
>         NEXT_PASS (pass_tail_calls);
> !       /* FIXME: If DCE is not run before checking for uninitialized uses,
>          we may get false warnings (e.g., testsuite/gcc.dg/uninit-5.c).
>          However, this also causes us to misdiagnose cases that should be
> !        real warnings (e.g., testsuite/gcc.dg/pr18501.c).
> !
> !        To fix the false positives in uninit-5.c, we would have to
> !        account for the predicates protecting the set and the use of each
> !        variable.  Using a representation like Gated Single Assignment
> !        may help.  */
>         /* Split critical edges before late uninit warning to reduce the
>            number of false positives from it.  */
>         NEXT_PASS (pass_split_crit_edges);
> --- 322,332 ----
>         NEXT_PASS (pass_fold_builtins);
>         NEXT_PASS (pass_optimize_widening_mul);
>         NEXT_PASS (pass_tail_calls);
> !       /* If DCE is not run before checking for uninitialized uses,
>          we may get false warnings (e.g., testsuite/gcc.dg/uninit-5.c).
>          However, this also causes us to misdiagnose cases that should be
> !        real warnings (e.g., testsuite/gcc.dg/pr18501.c).  */
> !       NEXT_PASS (pass_dce);
>         /* Split critical edges before late uninit warning to reduce the
>            number of false positives from it.  */
>         NEXT_PASS (pass_split_crit_edges);
> Index: gcc/match.pd
> ===================================================================
> *** gcc/match.pd        (revision 233241)
> --- gcc/match.pd        (working copy)
> *************** DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> *** 1717,1722 ****
> --- 1717,1745 ----
>    (simplify
>     (cnd @0 @1 (cnd @0 @2 @3))
>     (cnd @0 @1 @3))
> +  /* A ? B : (!A ? C : X) -> A ? B : C.  */
> +  /* ???  This matches embedded conditions open-coded because genmatch
> +     would generate matching code for conditions in separate stmts only.
> +     The following is still important to merge then and else arm cases
> +     from if-conversion.  */
> +  (simplify
> +   (cnd @0 @1 (cnd @2 @3 @4))
> +   (if (COMPARISON_CLASS_P (@0)
> +        && COMPARISON_CLASS_P (@2)
> +        && invert_tree_comparison
> +            (TREE_CODE (@0), HONOR_NANS (TREE_OPERAND (@0, 0))) == TREE_CODE (@2)
> +        && operand_equal_p (TREE_OPERAND (@0, 0), TREE_OPERAND (@2, 0), 0)
> +        && operand_equal_p (TREE_OPERAND (@0, 1), TREE_OPERAND (@2, 1), 0))
> +    (cnd @0 @1 @3)))
> +  (simplify
> +   (cnd @0 (cnd @1 @2 @3) @4)
> +   (if (COMPARISON_CLASS_P (@0)
> +        && COMPARISON_CLASS_P (@1)
> +        && invert_tree_comparison
> +            (TREE_CODE (@0), HONOR_NANS (TREE_OPERAND (@0, 0))) == TREE_CODE (@1)
> +        && operand_equal_p (TREE_OPERAND (@0, 0), TREE_OPERAND (@1, 0), 0)
> +        && operand_equal_p (TREE_OPERAND (@0, 1), TREE_OPERAND (@1, 1), 0))
> +    (cnd @0 @3 @4)))
>
>    /* A ? B : B -> B.  */
>    (simplify
> Index: gcc/testsuite/gcc.dg/uninit-22.c
> ===================================================================
> *** gcc/testsuite/gcc.dg/uninit-22.c    (revision 0)
> --- gcc/testsuite/gcc.dg/uninit-22.c    (revision 0)
> ***************
> *** 0 ****
> --- 1,69 ----
> + /* { dg-do compile } */
> + /* { dg-options "-O3 -Wuninitialized --param vect-max-version-for-alias-checks=20" } */
> +
> + #include <stdint.h>
> +
> + #define A1  2896 /* (1/sqrt(2))<<12 */
> + #define A2  2217
> + #define A3  3784
> + #define A4 -5352
> +
> + #define IDCT_TRANSFORM(dest,s0,s1,s2,s3,s4,s5,s6,s7,d0,d1,d2,d3,d4,d5,d6,d7,munge,src) {\
> +     const int a0 = (src)[s0] + (src)[s4]; \
> +     const int a1 = (src)[s0] - (src)[s4]; \
> +     const int a2 = (src)[s2] + (src)[s6]; \
> +     const int a3 = (A1*((src)[s2] - (src)[s6])) >> 11; \
> +     const int a4 = (src)[s5] + (src)[s3]; \
> +     const int a5 = (src)[s5] - (src)[s3]; \
> +     const int a6 = (src)[s1] + (src)[s7]; \
> +     const int a7 = (src)[s1] - (src)[s7]; \
> +     const int b0 = a4 + a6; \
> +     const int b1 = (A3*(a5 + a7)) >> 11; \
> +     const int b2 = ((A4*a5) >> 11) - b0 + b1; \
> +     const int b3 = (A1*(a6 - a4) >> 11) - b2; \
> +     const int b4 = ((A2*a7) >> 11) + b3 - b1; \
> +     (dest)[d0] = munge(a0+a2   +b0); \
> +     (dest)[d1] = munge(a1+a3-a2+b2); \
> +     (dest)[d2] = munge(a1-a3+a2+b3); \
> +     (dest)[d3] = munge(a0-a2   -b4); \
> +     (dest)[d4] = munge(a0-a2   +b4); \
> +     (dest)[d5] = munge(a1-a3+a2-b3); \
> +     (dest)[d6] = munge(a1+a3-a2-b2); \
> +     (dest)[d7] = munge(a0+a2   -b0); \
> + }
> +
> + #define MUNGE_NONE(x) (x)
> + #define IDCT_COL(dest,src) IDCT_TRANSFORM(dest,0,8,16,24,32,40,48,56,0,8,16,24,32,40,48,56,MUNGE_NONE,src)
> +
> + #define MUNGE_ROW(x) (((x) + 0x7F)>>8)
> + #define IDCT_ROW(dest,src) IDCT_TRANSFORM(dest,0,1,2,3,4,5,6,7,0,1,2,3,4,5,6,7,MUNGE_ROW,src)
> +
> + static inline void bink_idct_col(int *dest, const int32_t *src)
> + {
> +     if ((src[8]|src[16]|src[24]|src[32]|src[40]|src[48]|src[56])==0) {
> +         dest[0]  =
> +         dest[8]  =
> +         dest[16] =
> +         dest[24] =
> +         dest[32] =
> +         dest[40] =
> +         dest[48] =
> +         dest[56] = src[0];
> +     } else {
> +         IDCT_COL(dest, src);
> +     }
> + }
> +
> + int bink_idct_put_c(uint8_t *dest, int linesize, int32_t *block)
> + {
> +     int i;
> +     int temp[64];
> +     for (i = 0; i < 8; i++)
> +         bink_idct_col(&temp[i], &block[i]);
> +     for (i = 0; i < 8; i++) {
> +         IDCT_ROW( (&dest[i*linesize]), (&temp[8*i]) );
> +     }
> +
> +     return 0;
> + }
> +