On June 3, 2014 9:18:29 AM CEST, Thomas Preud'homme <thomas.preudho...@arm.com> wrote: >When a bitwise OR gimple statement has for operands SSA_NAME >initialized directly from memory source (no cast or other unary >statement intervening), a symbolic number will be used only partly >initialized. This was discovered by valgrind and reported as PR61328. >This patch fixes that by moving the initialization code in a separate >function that can be called from the two places that need it. There was >a problem of a field of a structure that was set in a function and the >value of this field was read before checking the result of the function >call. This would lead to missed optimization. > >ChangeLog is as follows: > >2014-05-29 Thomas Preud'homme <thomas.preudho...@arm.com> > > PR tree-optimization/61328 > * tree-ssa-math-opts.c (init_symbolic_number): Extract symbolic number > initialization from find_bswap_or_nop_1. > (find_bswap_or_nop_1): Test return value of find_bswap_or_nop_1 stored > in source_expr2 before using the size value the function sets. Also > make use of init_symbolic_number () in both the old place and > find_bswap_or_nop_load () to avoid reading uninitialized memory when > doing recursion in the GIMPLE_BINARY_RHS case. > >Ok for trunk?
OK. Thanks, Richard. >diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c >index d9afccf..6c26d6d 100644 >--- a/gcc/tree-ssa-math-opts.c >+++ b/gcc/tree-ssa-math-opts.c >@@ -1701,6 +1701,30 @@ verify_symbolic_number_p (struct symbolic_number >*n, gimple stmt) > return true; > } > >+/* Initialize the symbolic number N for the bswap pass from the base >element >+ SRC manipulated by the bitwise OR expression. */ >+ >+static bool >+init_symbolic_number (struct symbolic_number *n, tree src) >+{ >+ n->base_addr = n->offset = n->alias_set = n->vuse = NULL_TREE; >+ >+ /* Set up the symbolic number N by setting each byte to a value >between 1 and >+ the byte size of rhs1. The highest order byte is set to n->size >and the >+ lowest order byte to 1. */ >+ n->size = TYPE_PRECISION (TREE_TYPE (src)); >+ if (n->size % BITS_PER_UNIT != 0) >+ return false; >+ n->size /= BITS_PER_UNIT; >+ n->range = n->size; >+ n->n = CMPNOP; >+ >+ if (n->size < (int)sizeof (int64_t)) >+ n->n &= ((uint64_t)1 << (n->size * BITS_PER_UNIT)) - 1; >+ >+ return true; >+} >+ >/* Check if STMT might be a byte swap or a nop from a memory source and >returns >the answer. If so, REF is that memory source and the base of the memory >area >accessed and the offset of the access from that base are recorded in N. > */ >@@ -1713,26 +1737,27 @@ find_bswap_or_nop_load (gimple stmt, tree ref, >struct symbolic_number *n) > HOST_WIDE_INT bitsize, bitpos; > enum machine_mode mode; > int unsignedp, volatilep; >+ tree offset, base_addr; > > if (!gimple_assign_load_p (stmt) || gimple_has_volatile_ops (stmt)) > return false; > >- n->base_addr = get_inner_reference (ref, &bitsize, &bitpos, >&n->offset, >- &mode, &unsignedp, &volatilep, false); >+ base_addr = get_inner_reference (ref, &bitsize, &bitpos, &offset, >&mode, >+ &unsignedp, &volatilep, false); > >- if (TREE_CODE (n->base_addr) == MEM_REF) >+ if (TREE_CODE (base_addr) == MEM_REF) > { > offset_int bit_offset = 0; >- tree off = TREE_OPERAND (n->base_addr, 1); >+ tree off = TREE_OPERAND (base_addr, 1); > > if (!integer_zerop (off)) > { >- offset_int boff, coff = mem_ref_offset (n->base_addr); >+ offset_int boff, coff = mem_ref_offset (base_addr); > boff = wi::lshift (coff, LOG2_BITS_PER_UNIT); > bit_offset += boff; > } > >- n->base_addr = TREE_OPERAND (n->base_addr, 0); >+ base_addr = TREE_OPERAND (base_addr, 0); > >/* Avoid returning a negative bitpos as this may wreak havoc later. */ > if (wi::neg_p (bit_offset)) >@@ -1743,11 +1768,11 @@ find_bswap_or_nop_load (gimple stmt, tree ref, >struct symbolic_number *n) > Subtract it to BIT_OFFSET and add it (scaled) to OFFSET. */ > bit_offset -= tem; > tem = wi::arshift (tem, LOG2_BITS_PER_UNIT); >- if (n->offset) >- n->offset = size_binop (PLUS_EXPR, n->offset, >+ if (offset) >+ offset = size_binop (PLUS_EXPR, offset, > wide_int_to_tree (sizetype, tem)); > else >- n->offset = wide_int_to_tree (sizetype, tem); >+ offset = wide_int_to_tree (sizetype, tem); > } > > bitpos += bit_offset.to_shwi (); >@@ -1758,6 +1783,9 @@ find_bswap_or_nop_load (gimple stmt, tree ref, >struct symbolic_number *n) > if (bitsize % BITS_PER_UNIT) > return false; > >+ init_symbolic_number (n, ref); >+ n->base_addr = base_addr; >+ n->offset = offset; > n->bytepos = bitpos / BITS_PER_UNIT; > n->alias_set = reference_alias_ptr_type (ref); > n->vuse = gimple_vuse (stmt); >@@ -1816,28 +1844,12 @@ find_bswap_or_nop_1 (gimple stmt, struct >symbolic_number *n, int limit) > > /* If find_bswap_or_nop_1 returned NULL, STMT is a leaf node and > we have to initialize the symbolic number. */ >- if (!source_expr1 || gimple_assign_load_p (rhs1_stmt)) >+ if (!source_expr1) > { >- /* Set up the symbolic number N by setting each byte to a >- value between 1 and the byte size of rhs1. The highest >- order byte is set to n->size and the lowest order >- byte to 1. */ >- n->size = TYPE_PRECISION (TREE_TYPE (rhs1)); >- if (n->size % BITS_PER_UNIT != 0) >+ if (gimple_assign_load_p (stmt) >+ || !init_symbolic_number (n, rhs1)) > return NULL_TREE; >- n->size /= BITS_PER_UNIT; >- n->range = n->size; >- n->n = CMPNOP; >- >- if (n->size < (int)sizeof (int64_t)) >- n->n &= ((uint64_t)1 << >- (n->size * BITS_PER_UNIT)) - 1; >- >- if (!source_expr1) >- { >- n->base_addr = n->offset = n->alias_set = n->vuse = NULL_TREE; >- source_expr1 = rhs1; >- } >+ source_expr1 = rhs1; > } > > switch (code) >@@ -1915,7 +1927,10 @@ find_bswap_or_nop_1 (gimple stmt, struct >symbolic_number *n, int limit) > > source_expr2 = find_bswap_or_nop_1 (rhs2_stmt, &n2, limit - 1); > >- if (n1.size != n2.size || !source_expr2) >+ if (!source_expr2) >+ return NULL_TREE; >+ >+ if (n1.size != n2.size) > return NULL_TREE; > > if (!n1.vuse != !n2.vuse || > >Best regards, > >Thomas