From: Richard Biener Date: Thu, 4 Jun 2020 11:44:58 +0000 (+0200) Subject: middle-end/95493 - bogus MEM_ATTRS for variable array access X-Git-Tag: releases/gcc-10.2.0~159 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7d919c33fbd29a996326840dae3b5e093c3190f4;p=thirdparty%2Fgcc.git middle-end/95493 - bogus MEM_ATTRS for variable array access The following patch avoids keeping the inherited MEM_ATTRS when set_mem_attributes_minus_bitpos is called with a variable ARRAY_REF. The inherited ones may not reflect the correct offset and neither does the updated alias-set match the inherited MEM_EXPR. This all ends up confusing path-based alias-analysis, causing wrong-code. The fix is to stop not adopting a MEM_EXPR for certain kinds of expressions and instead handle everything we can. There's still the constant kind trees case which I'm too lazy to look into right now. I did refrain from adding SSA_NAME there and instead avoided calling set_mem_attributes_minus_bitpos when debug expression expansion ended up expanding a SSA definition RHS which should already have taken care of setting the appropriate MEM_ATTRS. It also avoids calling set_mem_attributes on the DECL_INITIAL of a CONST_DECL which seems pointless since there cannot be a sensible MEM_EXPR derived from that. We're overwriting both other possibly useful info, alias-set and alignment immediately so the following patch simply removes the call instead of making the function deal with even more (unexpected) trees that are not memory accesses. 2020-06-23 Richard Biener PR middle-end/95493 PR middle-end/95690 * cfgexpand.c (expand_debug_expr): Avoid calling set_mem_attributes_minus_bitpos when we were expanding an SSA name. * emit-rtl.c (set_mem_attributes_minus_bitpos): Remove ARRAY_REF special-casing, add CONSTRUCTOR to the set of special-cases we do not want MEM_EXPRs for. Assert we end up with reasonable MEM_EXPRs. * varasm.c (build_constant_desc): Remove set_mem_attributes call. * g++.dg/torture/pr95493.C: New testcase. * g++.dg/torture/pr95493-1.C: Likewise. * gfortran.dg/pr95690.f90: Likewise. --- diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index a7ec77d5c85e..c534cb31a472 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -4616,7 +4616,8 @@ expand_debug_expr (tree exp) op0 = copy_rtx (op0); if (op0 == orig_op0) op0 = shallow_copy_rtx (op0); - set_mem_attributes (op0, exp, 0); + if (TREE_CODE (tem) != SSA_NAME) + set_mem_attributes (op0, exp, 0); } if (known_eq (bitpos, 0) && mode == GET_MODE (op0)) diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c index c49628040f84..972512e81153 100644 --- a/gcc/emit-rtl.c +++ b/gcc/emit-rtl.c @@ -2065,8 +2065,10 @@ set_mem_attributes_minus_bitpos (rtx ref, tree t, int objectp, new_size = DECL_SIZE_UNIT (t); } - /* ??? If we end up with a constant here do record a MEM_EXPR. */ - else if (CONSTANT_CLASS_P (t)) + /* ??? If we end up with a constant or a descriptor do not + record a MEM_EXPR. */ + else if (CONSTANT_CLASS_P (t) + || TREE_CODE (t) == CONSTRUCTOR) ; /* If this is a field reference, record it. */ @@ -2080,59 +2082,12 @@ set_mem_attributes_minus_bitpos (rtx ref, tree t, int objectp, new_size = DECL_SIZE_UNIT (TREE_OPERAND (t, 1)); } - /* If this is an array reference, look for an outer field reference. */ - else if (TREE_CODE (t) == ARRAY_REF) - { - tree off_tree = size_zero_node; - /* We can't modify t, because we use it at the end of the - function. */ - tree t2 = t; - - do - { - tree index = TREE_OPERAND (t2, 1); - tree low_bound = array_ref_low_bound (t2); - tree unit_size = array_ref_element_size (t2); - - /* We assume all arrays have sizes that are a multiple of a byte. - First subtract the lower bound, if any, in the type of the - index, then convert to sizetype and multiply by the size of - the array element. */ - if (! integer_zerop (low_bound)) - index = fold_build2 (MINUS_EXPR, TREE_TYPE (index), - index, low_bound); - - off_tree = size_binop (PLUS_EXPR, - size_binop (MULT_EXPR, - fold_convert (sizetype, - index), - unit_size), - off_tree); - t2 = TREE_OPERAND (t2, 0); - } - while (TREE_CODE (t2) == ARRAY_REF); - - if (DECL_P (t2) - || (TREE_CODE (t2) == COMPONENT_REF - /* For trailing arrays t2 doesn't have a size that - covers all valid accesses. */ - && ! array_at_struct_end_p (t))) - { - attrs.expr = t2; - attrs.offset_known_p = false; - if (poly_int_tree_p (off_tree, &attrs.offset)) - { - attrs.offset_known_p = true; - apply_bitpos = bitpos; - } - } - /* Else do not record a MEM_EXPR. */ - } - - /* If this is an indirect reference, record it. */ - else if (TREE_CODE (t) == MEM_REF - || TREE_CODE (t) == TARGET_MEM_REF) + /* Else record it. */ + else { + gcc_assert (handled_component_p (t) + || TREE_CODE (t) == MEM_REF + || TREE_CODE (t) == TARGET_MEM_REF); attrs.expr = t; attrs.offset_known_p = true; attrs.offset = 0; diff --git a/gcc/testsuite/g++.dg/torture/pr95493-1.C b/gcc/testsuite/g++.dg/torture/pr95493-1.C new file mode 100644 index 000000000000..907d191ebfeb --- /dev/null +++ b/gcc/testsuite/g++.dg/torture/pr95493-1.C @@ -0,0 +1,95 @@ +// { dg-do run { target c++11 } } +// PR rtl-optimization/95493 comment 8 + +#include +#include + +struct Point +{ + std::array array; + + Point(int x, int y, int z) : array{x, y, z} {} + + Point(const Point & other) : array{other.array} {} // OK if commented + //Point(const Point &) = default; // OK + + //Point(Point && other) = default; // OK + + int operator[] (std::size_t i) const { return array[i]; } + int& operator[] (std::size_t i) { return array[i]; } +}; + +//using Point = std::array; // OK + +struct Cell +{ + Point point; + Cell(Point const& pt) : point(pt) {} + int operator[] (std::size_t i) const { return point[i]; } + int& operator[] (std::size_t i) { return point[i]; } +}; + +//using Cell = Point; // OK + +std::ostream & operator<< (std::ostream & out, Cell const& object) +//std::ostream & operator<< (std::ostream & out, Cell object) // Fails with f2 too +{ + for ( std::size_t i = 0; i < 3; ++i ) + out << object[ i ] << " "; + return out; +} + + +struct DirIterator +{ + std::size_t dir; + Cell cell; + + DirIterator(Cell c) + : dir(0), cell(c) + { + find(); // OK if commented + } + + void find() + { + //while (dir < 3) // Fails with f2 too + while (dir < 3 && (cell[dir] % 2) == 0) + ++dir; + } +}; + +Cell uIncident(Cell c, std::size_t k) +//Cell uIncident(Cell& c, std::size_t k) // OK +{ + --c[k]; + return c; +} + +Cell uSpel(Point p) +{ + for (std::size_t i = 0; i < 3; ++i) + p[i] += p[i] + 1; + return Cell(p); +} + + +int main () +{ + Cell c = uSpel(Point{0, 0, 0}); // Fails + //Cell c( Point(1, 1, 1) ); // OK + + auto q = DirIterator( c ); + + Cell f1 = uIncident( c, q.dir ); // Fails + //Cell f1 = uIncident( c, 0 ); // OK + + Cell f2 = f1; // f2 is the right cell that f1 should be + + std::cout << "q.dir = " << q.dir << " ; f1 = " << f1 << " ; f2 = " << f2 << std::endl; + //std::cout << "q.dir = " << q.dir << " ; f1 = " << f1[0] << " " << f1[1] << " " << f1[2] << " ; f2 = " << f2[0] << " " << f2[1] << " " << f2[2] << std::endl; // OK + + for (int i = 0; i < 3; ++i) + if (f1[i] != f2[i]) + __builtin_abort(); +} diff --git a/gcc/testsuite/g++.dg/torture/pr95493.C b/gcc/testsuite/g++.dg/torture/pr95493.C new file mode 100644 index 000000000000..5e05056854dc --- /dev/null +++ b/gcc/testsuite/g++.dg/torture/pr95493.C @@ -0,0 +1,62 @@ +// { dg-do run } +// { dg-additional-options "-std=c++17" } + +struct verify +{ + const bool m_failed = false; + + [[gnu::noinline]] void print_hex(const void* x, int n) const + { + const auto* bytes = static_cast(x); + for (int i = 0; i < n; ++i) + __builtin_printf((i && i % 4 == 0) ? "'%02x" : "%02x", bytes[i]); + __builtin_printf("\n"); + } + + template + verify(bool ok, const Ts&... extra_info) : m_failed(!ok) + { + if (m_failed) + (print_hex(&extra_info, sizeof(extra_info)), ...); + } + + ~verify() + { + if (m_failed) + __builtin_abort(); + } +}; + +using K [[gnu::vector_size(16)]] = int; + +int +main() +{ + int count = 1; + asm("" : "+m"(count)); + verify(count == 1, 0, "", 0); + + { + struct SW + { + K d; + }; + struct + { + SW d; + } xx; + SW& x = xx.d; + x = SW(); // [0, 0, 0, 0] + for (int i = 3; i >= 2; --i) + { + x.d[i] = -1; // [0, 0, 0, -1] ... + int a = [](K y) { + for (int j = 0; j < 4; ++j) + if (y[j] != 0) + return j; + return -1; + }(x.d); + verify(a == i, 0, 0, 0, 0, i, x); + } + } +} diff --git a/gcc/testsuite/gfortran.dg/pr95690.f90 b/gcc/testsuite/gfortran.dg/pr95690.f90 new file mode 100644 index 000000000000..2da08912a4d5 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/pr95690.f90 @@ -0,0 +1,9 @@ +! { dg-do compile } +module m +contains + subroutine s + print *, (erfc) ! { dg-error "not a floating constant" } + end + function erfc() + end +end diff --git a/gcc/varasm.c b/gcc/varasm.c index 271a67abf562..5bf4e96a7739 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -3425,7 +3425,6 @@ build_constant_desc (tree exp) TREE_CONSTANT_POOL_ADDRESS_P (symbol) = 1; rtl = gen_const_mem (TYPE_MODE (TREE_TYPE (exp)), symbol); - set_mem_attributes (rtl, exp, 1); set_mem_alias_set (rtl, 0); /* Putting EXP into the literal pool might have imposed a different