| Message ID | alpine.DEB.2.02.1403022113150.1515@stedding.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
Ping http://gcc.gnu.org/ml/gcc-patches/2014-03/msg00051.html On Sun, 2 Mar 2014, Marc Glisse wrote: > Hello, > > inlining operator new (with LTO or otherwise), I noticed that it has a > complicated implementation, which makes it hard to use this inlined code for > optimizations. This patch does two things: > > 1) there are 2 calls to malloc, I am turning them into just one. At -Os, it > does not change the generated code (RTL optimizers manage to merge the calls > to malloc). At other levels (-O2, -O3, and especially with -g) it gives a > smaller object file. And with just one malloc, some optimizations become much > easier (see my recent calloc patch for instance). > > 2) malloc is predicted to return null 19 times out of 20 because of the loop > (that didn't change with the patch), so I am adding __builtin_expect to let > gcc optimize the fast path. > > Further discussion: > > a) I didn't add __builtin_expect for the test (sz == 0), it didn't change the > generated code in my limited test. I was wondering if this test is necessary > (new doesn't seem to ever call operator new(0)) or could be moved to operator > new[] (new type[0] does call operator new[](0)), but since one can call > operator new directly, it has to be protected indeed, so let's forget this > point ;-) > (too bad malloc is replacable, so we can't use the fact that glibc already > does the right thing) > > b) I have a bit of trouble parsing the standard. Is the nothrow operator new > supposed to call the regular operator new? In particular, if a user replaces > only the throwing operator new, should the nothrow operator new automatically > call that function? That's not what we are currently doing (and it would be a > perf regression). > > "Required behavior: Return a non-null pointer to suitably aligned storage > (3.7.4), or else return a null pointer. This nothrow version of operator new > returns a pointer obtained as if acquired from the (possibly replaced) > ordinary version. This requirement is binding on a replacement version of > this function. > > Default behavior: Calls operator new(size). If the call returns normally, > returns the result of that call. Otherwise, returns a null pointer." > > > > Passes bootstrap+testsuite on x86_64-linux-gnu. Stage 1? > > 2014-03-03 Marc Glisse <marc.glisse@inria.fr> > > * libsupc++/new_op.cc: Factor the calls to malloc, use > __builtin_expect. > * libsupc++/new_opnt.cc: Likewise. > >
Ping. On Tue, 15 Apr 2014, Marc Glisse wrote: > Ping > http://gcc.gnu.org/ml/gcc-patches/2014-03/msg00051.html > > > On Sun, 2 Mar 2014, Marc Glisse wrote: > >> Hello, >> >> inlining operator new (with LTO or otherwise), I noticed that it has a >> complicated implementation, which makes it hard to use this inlined code >> for optimizations. This patch does two things: >> >> 1) there are 2 calls to malloc, I am turning them into just one. At -Os, it >> does not change the generated code (RTL optimizers manage to merge the >> calls to malloc). At other levels (-O2, -O3, and especially with -g) it >> gives a smaller object file. And with just one malloc, some optimizations >> become much easier (see my recent calloc patch for instance). >> >> 2) malloc is predicted to return null 19 times out of 20 because of the >> loop (that didn't change with the patch), so I am adding __builtin_expect >> to let gcc optimize the fast path. >> >> Further discussion: >> >> a) I didn't add __builtin_expect for the test (sz == 0), it didn't change >> the generated code in my limited test. I was wondering if this test is >> necessary (new doesn't seem to ever call operator new(0)) or could be moved >> to operator new[] (new type[0] does call operator new[](0)), but since one >> can call operator new directly, it has to be protected indeed, so let's >> forget this point ;-) >> (too bad malloc is replacable, so we can't use the fact that glibc already >> does the right thing) >> >> b) I have a bit of trouble parsing the standard. Is the nothrow operator >> new supposed to call the regular operator new? In particular, if a user >> replaces only the throwing operator new, should the nothrow operator new >> automatically call that function? That's not what we are currently doing >> (and it would be a perf regression). >> >> "Required behavior: Return a non-null pointer to suitably aligned storage >> (3.7.4), or else return a null pointer. This nothrow version of operator >> new returns a pointer obtained as if acquired from the (possibly replaced) >> ordinary version. This requirement is binding on a replacement version of >> this function. >> >> Default behavior: Calls operator new(size). If the call returns normally, >> returns the result of that call. Otherwise, returns a null pointer." >> >> >> >> Passes bootstrap+testsuite on x86_64-linux-gnu. Stage 1? >> >> 2014-03-03 Marc Glisse <marc.glisse@inria.fr> >> >> * libsupc++/new_op.cc: Factor the calls to malloc, use >> __builtin_expect. >> * libsupc++/new_opnt.cc: Likewise.
On 17/05/14 18:30 +0200, Marc Glisse wrote: >Ping. > >On Tue, 15 Apr 2014, Marc Glisse wrote: > >>Ping >>http://gcc.gnu.org/ml/gcc-patches/2014-03/msg00051.html The patch is OK for trunk - sorry for forgetting about it.
Index: libsupc++/new_op.cc =================================================================== --- libsupc++/new_op.cc (revision 208255) +++ libsupc++/new_op.cc (working copy) @@ -39,22 +39,21 @@ extern "C" void *malloc (std::size_t); #endif _GLIBCXX_WEAK_DEFINITION void * operator new (std::size_t sz) _GLIBCXX_THROW (std::bad_alloc) { void *p; /* malloc (0) is unpredictable; avoid it. */ if (sz == 0) sz = 1; - p = (void *) malloc (sz); - while (p == 0) + + while (__builtin_expect ((p = malloc (sz)) == 0, false)) { new_handler handler = std::get_new_handler (); if (! handler) _GLIBCXX_THROW_OR_ABORT(bad_alloc()); handler (); - p = (void *) malloc (sz); } return p; } Index: libsupc++/new_opnt.cc =================================================================== --- libsupc++/new_opnt.cc (revision 208255) +++ libsupc++/new_opnt.cc (working copy) @@ -32,30 +32,28 @@ using std::bad_alloc; extern "C" void *malloc (std::size_t); _GLIBCXX_WEAK_DEFINITION void * operator new (std::size_t sz, const std::nothrow_t&) _GLIBCXX_USE_NOEXCEPT { void *p; /* malloc (0) is unpredictable; avoid it. */ if (sz == 0) sz = 1; - p = (void *) malloc (sz); - while (p == 0) + + while (__builtin_expect ((p = malloc (sz)) == 0, false)) { new_handler handler = std::get_new_handler (); if (! handler) return 0; __try { handler (); } __catch(const bad_alloc&) { return 0; } - - p = (void *) malloc (sz); } return p; }