| Message ID | 5072F64A.6030509@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hello Florian, Let's CC Jason for this optimization patch. Florian Weimer <fweimer@redhat.com> a écrit: > If the size of the inner array elements is 1 and we do not need a > cookie, we do not need to insert an overflow check. This applies to > the relatively frequent new char[n] case. I just have one question for own education. Regarding: @@ -2450,7 +2450,13 @@ if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type)) size = size_binop (PLUS_EXPR, size, cookie_size); else - cookie_size = NULL_TREE; + { + cookie_size = NULL_TREE; + /* No size arithmetic necessary, so the size check is + not needed. */ + if (outer_nelts_check != NULL && inner_size == double_int_one) + outer_nelts_check = NULL_TREE; + } I couldn't find where in code is TYPE_VEC_NEW_USES_COOKIE is set. Is it still used?
On 10/10/2012 06:02 PM, Dodji Seketeli wrote: > I just have one question for own education. > > Regarding: > > @@ -2450,7 +2450,13 @@ > if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type)) > size = size_binop (PLUS_EXPR, size, cookie_size); > else > - cookie_size = NULL_TREE; > + { > + cookie_size = NULL_TREE; > + /* No size arithmetic necessary, so the size check is > + not needed. */ > + if (outer_nelts_check != NULL && inner_size == double_int_one) > + outer_nelts_check = NULL_TREE; > + } > > I couldn't find where in code is TYPE_VEC_NEW_USES_COOKIE is set. Is it > still used? It's set in gcc/cp/class.c: 5191 /* Figure out whether or not we will need a cookie when dynamically 5192 allocating an array of this type. */ 5193 TYPE_LANG_SPECIFIC (t)->u.c.vec_new_uses_cookie 5194 = type_requires_array_cookie (t); I'm not sure if we've got proper test coverage for the concrete cookie value, but the test case I've included implicitly check if there's a cookie if there's a non-trivial destructor.
Florian Weimer <fweimer@redhat.com> a écrit: > On 10/10/2012 06:02 PM, Dodji Seketeli wrote: > >> I just have one question for own education. >> >> Regarding: >> >> @@ -2450,7 +2450,13 @@ >> if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type)) >> size = size_binop (PLUS_EXPR, size, cookie_size); >> else >> - cookie_size = NULL_TREE; >> + { >> + cookie_size = NULL_TREE; >> + /* No size arithmetic necessary, so the size check is >> + not needed. */ >> + if (outer_nelts_check != NULL && inner_size == double_int_one) >> + outer_nelts_check = NULL_TREE; >> + } >> >> I couldn't find where in code is TYPE_VEC_NEW_USES_COOKIE is set. Is it >> still used? > > It's set in gcc/cp/class.c: > > 5191 /* Figure out whether or not we will need a cookie when > dynamically > 5192 allocating an array of this type. */ > 5193 TYPE_LANG_SPECIFIC (t)->u.c.vec_new_uses_cookie > 5194 = type_requires_array_cookie (t); Ah, I see. So maybe that statement should be trivially replaced by: TYPE_VEC_NEW_USES_COOKIE (t) = type_requires_array_cookie (t); for better maintainability? > I'm not sure if we've got proper test coverage for the concrete cookie > value, but the test case I've included implicitly check if there's a > cookie if there's a non-trivial destructor. I see, Thanks.
On Mon, Oct 08, 2012 at 05:50:34PM +0200, Florian Weimer wrote: > gcc/: > > 2012-10-08 Florian Weimer <fweimer@redhat.com> > > * init.c (build_new_1): Do not check for arithmetic overflow if > inner array size is 1. > > gcc/testsuite/: > > 2012-10-08 Florian Weimer <fweimer@redhat.com> > > * g++.dg/init/new40.C: New. > @@ -2450,7 +2450,13 @@ > if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type)) > size = size_binop (PLUS_EXPR, size, cookie_size); > else > - cookie_size = NULL_TREE; > + { > + cookie_size = NULL_TREE; > + /* No size arithmetic necessary, so the size check is > + not needed. */ > + if (outer_nelts_check != NULL && inner_size == double_int_one) > + outer_nelts_check = NULL_TREE; > + } I don't see any == double_int_one (or zero) comparisons in grep, so I'd say inner_size.is_one () should be used instead (which is used pretty frequently). Ditto in the second spot. Otherwise the patch looks good to me, but I'd like Jason to chime in too. > /* Perform the overflow check. */ > if (outer_nelts_check != NULL_TREE) > size = fold_build3 (COND_EXPR, sizetype, outer_nelts_check, > @@ -2486,7 +2492,13 @@ > /* Use a global operator new. */ > /* See if a cookie might be required. */ > if (!(array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type))) > - cookie_size = NULL_TREE; > + { > + cookie_size = NULL_TREE; > + /* No size arithmetic necessary, so the size check is > + not needed. */ > + if (outer_nelts_check != NULL && inner_size == double_int_one) > + outer_nelts_check = NULL_TREE; > + } > > alloc_call = build_operator_new_call (fnname, placement, > &size, &cookie_size, Jakub
gcc/: 2012-10-08 Florian Weimer <fweimer@redhat.com> * init.c (build_new_1): Do not check for arithmetic overflow if inner array size is 1. gcc/testsuite/: 2012-10-08 Florian Weimer <fweimer@redhat.com> * g++.dg/init/new40.C: New. Index: gcc/cp/ChangeLog =================================================================== --- gcc/cp/ChangeLog (revision 192206) +++ gcc/cp/ChangeLog (working copy) @@ -1,3 +1,8 @@ +2012-10-08 Florian Weimer <fweimer@redhat.com> + + * init.c (build_new_1): Do not check for arithmetic overflow if + inner array size is 1. + 2012-10-08 Dodji Seketeli <dodji@redhat.com> PR c++/53528 C++11 attribute support Index: gcc/cp/init.c =================================================================== --- gcc/cp/init.c (revision 192206) +++ gcc/cp/init.c (working copy) @@ -2184,6 +2184,8 @@ bool outer_nelts_from_type = false; double_int inner_nelts_count = double_int_one; tree alloc_call, alloc_expr; + /* Size of the inner array elements. */ + double_int inner_size; /* The address returned by the call to "operator new". This node is a VAR_DECL and is therefore reusable. */ tree alloc_node; @@ -2345,8 +2347,6 @@ double_int max_size = double_int_one.llshift (TYPE_PRECISION (sizetype) - 1, HOST_BITS_PER_DOUBLE_INT); - /* Size of the inner array elements. */ - double_int inner_size; /* Maximum number of outer elements which can be allocated. */ double_int max_outer_nelts; tree max_outer_nelts_tree; @@ -2450,7 +2450,13 @@ if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type)) size = size_binop (PLUS_EXPR, size, cookie_size); else - cookie_size = NULL_TREE; + { + cookie_size = NULL_TREE; + /* No size arithmetic necessary, so the size check is + not needed. */ + if (outer_nelts_check != NULL && inner_size == double_int_one) + outer_nelts_check = NULL_TREE; + } /* Perform the overflow check. */ if (outer_nelts_check != NULL_TREE) size = fold_build3 (COND_EXPR, sizetype, outer_nelts_check, @@ -2486,7 +2492,13 @@ /* Use a global operator new. */ /* See if a cookie might be required. */ if (!(array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type))) - cookie_size = NULL_TREE; + { + cookie_size = NULL_TREE; + /* No size arithmetic necessary, so the size check is + not needed. */ + if (outer_nelts_check != NULL && inner_size == double_int_one) + outer_nelts_check = NULL_TREE; + } alloc_call = build_operator_new_call (fnname, placement, &size, &cookie_size, Index: gcc/testsuite/ChangeLog =================================================================== --- gcc/testsuite/ChangeLog (revision 192206) +++ gcc/testsuite/ChangeLog (working copy) @@ -1,3 +1,7 @@ +2012-10-08 Florian Weimer <fweimer@redhat.com> + + * g++.dg/init/new40.C: New. + 2012-10-08 Oleg Endo <olegendo@gcc.gnu.org> PR target/54685 Index: gcc/testsuite/g++.dg/init/new40.C =================================================================== --- gcc/testsuite/g++.dg/init/new40.C (revision 0) +++ gcc/testsuite/g++.dg/init/new40.C (working copy) @@ -0,0 +1,112 @@ +// Testcase for overflow handling in operator new[]. +// Optimization of unnecessary overflow checks. +// { dg-do run } + +#include <assert.h> +#include <stdlib.h> +#include <stdexcept> + +static size_t magic_allocation_size + = 1 + (size_t (1) << (sizeof (size_t) * 8 - 1)); + +struct exc : std::bad_alloc { +}; + +static size_t expected_size; + +struct pod_with_new { + char ch; + void *operator new[] (size_t sz) + { + if (sz != expected_size) + abort (); + throw exc (); + } +}; + +struct with_new { + char ch; + with_new () { } + ~with_new () { } + void *operator new[] (size_t sz) + { + if (sz != size_t (-1)) + abort (); + throw exc (); + } +}; + +struct non_pod { + char ch; + non_pod () { } + ~non_pod () { } +}; + +void * +operator new (size_t sz) _GLIBCXX_THROW (std::bad_alloc) +{ + if (sz != expected_size) + abort (); + throw exc (); +} + +int +main () +{ + if (sizeof (pod_with_new) == 1) + expected_size = magic_allocation_size; + else + expected_size = -1; + + try { + new pod_with_new[magic_allocation_size]; + abort (); + } catch (exc &) { + } + + if (sizeof (with_new) == 1) + expected_size = magic_allocation_size; + else + expected_size = -1; + + try { + new with_new[magic_allocation_size]; + abort (); + } catch (exc &) { + } + + expected_size = magic_allocation_size; + try { + new char[magic_allocation_size]; + abort (); + } catch (exc &) { + } + + expected_size = -1; + + try { + new pod_with_new[magic_allocation_size][2]; + abort (); + } catch (exc &) { + } + + try { + new with_new[magic_allocation_size][2]; + abort (); + } catch (exc &) { + } + + try { + new char[magic_allocation_size][2]; + abort (); + } catch (exc &) { + } + + try { + new non_pod[magic_allocation_size]; + abort (); + } catch (exc &) { + } + + return 0; +}