[PATCH] Add __attribute__((malloc) to allocator and remove unused code

[PATCH] Add __attribute__((malloc) to allocator and remove unused code

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

As discussed at https://gcc.gnu.org/ml/libstdc++/2018-01/msg00073.html
we can simplify the allocator function for valarray memory. I also
noticed that the _Array(size_t) constructor is never used.

	* include/bits/valarray_array.h (__valarray_get_memory): Remove.
	(__valarray_get_storage): Call operator new directly. Remove ignored
	top-level restrict qualifier and add malloc attribute instead.
	(_Array<_Tp>::_Array(size_t)): Remove unused constructor.

Tested powerpc64le-linux, committed to trunk.



[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2125 bytes --]

commit 71983b7d0901159af2bca65af783460721fc0a76
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon May 14 16:02:18 2018 +0100

    Add __attribute__((malloc) to allocator and remove unused code
    
            * include/bits/valarray_array.h (__valarray_get_memory): Remove.
            (__valarray_get_storage): Call operator new directly. Remove ignored
            top-level restrict qualifier and add malloc attribute instead.
            (_Array<_Tp>::_Array(size_t)): Remove unused constructor.

diff --git a/libstdc++-v3/include/bits/valarray_array.h b/libstdc++-v3/include/bits/valarray_array.h
index 07f38ed03ed..6759d6003e9 100644
--- a/libstdc++-v3/include/bits/valarray_array.h
+++ b/libstdc++-v3/include/bits/valarray_array.h
@@ -47,18 +47,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   // Helper functions on raw pointers
   //
 
-  // We get memory by the old fashion way
-  inline void*
-  __valarray_get_memory(size_t __n)
-  { return operator new(__n); }
+  // We get memory the old fashioned way
+  template<typename _Tp>
+    _Tp*
+    __valarray_get_storage(size_t) __attribute__((__malloc__));
 
   template<typename _Tp>
-    inline _Tp*__restrict__
+    inline _Tp*
     __valarray_get_storage(size_t __n)
-    {
-      return static_cast<_Tp*__restrict__>
-	(std::__valarray_get_memory(__n * sizeof(_Tp)));
-    }
+    { return static_cast<_Tp*>(operator new(__n * sizeof(_Tp))); }
 
   // Return memory to the system
   inline void
@@ -410,7 +407,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _Tp>
     struct _Array
     {
-      explicit _Array(size_t);
       explicit _Array(_Tp* const __restrict__);
       explicit _Array(const valarray<_Tp>&);
       _Array(const _Tp* __restrict__, size_t);
@@ -503,12 +499,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		    __dst._M_data, __j._M_data);
     }
 
-  template<typename _Tp>
-    inline
-    _Array<_Tp>::_Array(size_t __n)
-    : _M_data(__valarray_get_storage<_Tp>(__n))
-    { std::__valarray_default_construct(_M_data, _M_data + __n); }
-
   template<typename _Tp>
     inline
     _Array<_Tp>::_Array(_Tp* const __restrict__ __p)

Re: [PATCH] Add __attribute__((malloc) to allocator and remove unused code

[Marc Glisse]

On Mon, 14 May 2018, Jonathan Wakely wrote:

> As discussed at https://gcc.gnu.org/ml/libstdc++/2018-01/msg00073.html
> we can simplify the allocator function for valarray memory. I also
> noticed that the _Array(size_t) constructor is never used.
>
> 	* include/bits/valarray_array.h (__valarray_get_memory): Remove.
> 	(__valarray_get_storage): Call operator new directly. Remove ignored
> 	top-level restrict qualifier and add malloc attribute instead.

I am trying to understand the point of adding this attribute. The function 
is just

{ return static_cast<_Tp*>(operator new(__n * sizeof(_Tp))); }

The idea is that it isn't safe (? see PR 23383) to mark operator new with 
the attribute, but it is safe for this particular use?

When optimizing, I certainly hope this trivial function gets inlined, and 
then the attribute is lost (should the inliner add 'restrict' when 
inlining a function with attribute malloc?) and all that matters is 
operator new.

-- 
Marc Glisse

Re: [PATCH] Add __attribute__((malloc) to allocator and remove unused code

[Jonathan Wakely]

On 17/05/18 12:54 +0200, Marc Glisse wrote:
>On Mon, 14 May 2018, Jonathan Wakely wrote:
>
>>As discussed at https://gcc.gnu.org/ml/libstdc++/2018-01/msg00073.html
>>we can simplify the allocator function for valarray memory. I also
>>noticed that the _Array(size_t) constructor is never used.
>>
>>	* include/bits/valarray_array.h (__valarray_get_memory): Remove.
>>	(__valarray_get_storage): Call operator new directly. Remove ignored
>>	top-level restrict qualifier and add malloc attribute instead.
>
>I am trying to understand the point of adding this attribute. The 
>function is just
>
>{ return static_cast<_Tp*>(operator new(__n * sizeof(_Tp))); }
>
>The idea is that it isn't safe (? see PR 23383) to mark operator new 
>with the attribute, but it is safe for this particular use?

I'd forgotten about that (I was assuming the compiler doesn't need to
be told about the properties of operator new, because they're defined
by the language). We can remove the attribute.


>When optimizing, I certainly hope this trivial function gets inlined, 
>and then the attribute is lost (should the inliner add 'restrict' when 
>inlining a function with attribute malloc?) and all that matters is 
>operator new.

Re: [PATCH] Add __attribute__((malloc) to allocator and remove unused code

[Marc Glisse]

On Thu, 17 May 2018, Jonathan Wakely wrote:

> On 17/05/18 12:54 +0200, Marc Glisse wrote:
>> On Mon, 14 May 2018, Jonathan Wakely wrote:
>> 
>>> As discussed at https://gcc.gnu.org/ml/libstdc++/2018-01/msg00073.html
>>> we can simplify the allocator function for valarray memory. I also
>>> noticed that the _Array(size_t) constructor is never used.
>>>
>>> 	* include/bits/valarray_array.h (__valarray_get_memory): Remove.
>>> 	(__valarray_get_storage): Call operator new directly. Remove ignored
>>> 	top-level restrict qualifier and add malloc attribute instead.
>> 
>> I am trying to understand the point of adding this attribute. The function 
>> is just
>> 
>> { return static_cast<_Tp*>(operator new(__n * sizeof(_Tp))); }
>> 
>> The idea is that it isn't safe (? see PR 23383) to mark operator new with 
>> the attribute, but it is safe for this particular use?
>
> I'd forgotten about that (I was assuming the compiler doesn't need to
> be told about the properties of operator new, because they're defined
> by the language). We can remove the attribute.

I am not necessarily asking to remove it. I don't have a good 
understanding of what would break if we marked operator new with the 
attribute, so I have no idea if those reasons also apply for this use in 
valarray.

>> When optimizing, I certainly hope this trivial function gets inlined, and 
>> then the attribute is lost (should the inliner add 'restrict' when inlining 
>> a function with attribute malloc?) and all that matters is operator new.

If we determine that using the attribute here but not on operator new is 
the right choice, then I believe we need some middle-end tweaks so it 
isn't ignored.

-- 
Marc Glisse

Re: [PATCH] Add __attribute__((malloc) to allocator and remove unused code

[Richard Biener]

On Thu, May 17, 2018 at 1:14 PM Marc Glisse <marc.glisse@inria.fr> wrote:

> On Thu, 17 May 2018, Jonathan Wakely wrote:

> > On 17/05/18 12:54 +0200, Marc Glisse wrote:
> >> On Mon, 14 May 2018, Jonathan Wakely wrote:
> >>
> >>> As discussed at https://gcc.gnu.org/ml/libstdc++/2018-01/msg00073.html
> >>> we can simplify the allocator function for valarray memory. I also
> >>> noticed that the _Array(size_t) constructor is never used.
> >>>
> >>>     * include/bits/valarray_array.h (__valarray_get_memory): Remove.
> >>>     (__valarray_get_storage): Call operator new directly. Remove
ignored
> >>>     top-level restrict qualifier and add malloc attribute instead.
> >>
> >> I am trying to understand the point of adding this attribute. The
function
> >> is just
> >>
> >> { return static_cast<_Tp*>(operator new(__n * sizeof(_Tp))); }
> >>
> >> The idea is that it isn't safe (? see PR 23383) to mark operator new
with
> >> the attribute, but it is safe for this particular use?
> >
> > I'd forgotten about that (I was assuming the compiler doesn't need to
> > be told about the properties of operator new, because they're defined
> > by the language). We can remove the attribute.

> I am not necessarily asking to remove it. I don't have a good
> understanding of what would break if we marked operator new with the
> attribute, so I have no idea if those reasons also apply for this use in
> valarray.

> >> When optimizing, I certainly hope this trivial function gets inlined,
and
> >> then the attribute is lost (should the inliner add 'restrict' when
inlining
> >> a function with attribute malloc?) and all that matters is operator
new.

> If we determine that using the attribute here but not on operator new is
> the right choice, then I believe we need some middle-end tweaks so it
> isn't ignored.

We don't have a good way to do this.  Your suggestion of adding restrict
would work if it were not that only function-scope restrict uses are later
handled...

Richard.

> --
> Marc Glisse

Re: [PATCH] Add __attribute__((malloc) to allocator and remove unused code

[Marc Glisse]

On Thu, 17 May 2018, Richard Biener wrote:

> On Thu, May 17, 2018 at 1:14 PM Marc Glisse <marc.glisse@inria.fr> wrote:
>
>> On Thu, 17 May 2018, Jonathan Wakely wrote:
>
>>> On 17/05/18 12:54 +0200, Marc Glisse wrote:
>>>> On Mon, 14 May 2018, Jonathan Wakely wrote:
>>>>
>>>>> As discussed at https://gcc.gnu.org/ml/libstdc++/2018-01/msg00073.html
>>>>> we can simplify the allocator function for valarray memory. I also
>>>>> noticed that the _Array(size_t) constructor is never used.
>>>>>
>>>>>     * include/bits/valarray_array.h (__valarray_get_memory): Remove.
>>>>>     (__valarray_get_storage): Call operator new directly. Remove
> ignored
>>>>>     top-level restrict qualifier and add malloc attribute instead.
>>>>
>>>> I am trying to understand the point of adding this attribute. The
> function
>>>> is just
>>>>
>>>> { return static_cast<_Tp*>(operator new(__n * sizeof(_Tp))); }
>>>>
>>>> The idea is that it isn't safe (? see PR 23383) to mark operator new
> with
>>>> the attribute, but it is safe for this particular use?
>>>
>>> I'd forgotten about that (I was assuming the compiler doesn't need to
>>> be told about the properties of operator new, because they're defined
>>> by the language). We can remove the attribute.
>
>> I am not necessarily asking to remove it. I don't have a good
>> understanding of what would break if we marked operator new with the
>> attribute, so I have no idea if those reasons also apply for this use in
>> valarray.
>
>>>> When optimizing, I certainly hope this trivial function gets inlined,
> and
>>>> then the attribute is lost (should the inliner add 'restrict' when
> inlining
>>>> a function with attribute malloc?) and all that matters is operator
> new.
>
>> If we determine that using the attribute here but not on operator new is
>> the right choice, then I believe we need some middle-end tweaks so it
>> isn't ignored.
>
> We don't have a good way to do this.  Your suggestion of adding restrict
> would work if it were not that only function-scope restrict uses are later
> handled...

This seems extremely similar to the issue of inlining functions with 
restrict arguments.

I have written a PR, but it is probably not worth submitting.

-- 
Marc Glisse

Re: [PATCH] Add __attribute__((malloc) to allocator and remove unused code

[Richard Biener]

On Thu, May 17, 2018 at 1:42 PM Marc Glisse <marc.glisse@inria.fr> wrote:

> On Thu, 17 May 2018, Richard Biener wrote:

> > On Thu, May 17, 2018 at 1:14 PM Marc Glisse <marc.glisse@inria.fr>
wrote:
> >
> >> On Thu, 17 May 2018, Jonathan Wakely wrote:
> >
> >>> On 17/05/18 12:54 +0200, Marc Glisse wrote:
> >>>> On Mon, 14 May 2018, Jonathan Wakely wrote:
> >>>>
> >>>>> As discussed at
https://gcc.gnu.org/ml/libstdc++/2018-01/msg00073.html
> >>>>> we can simplify the allocator function for valarray memory. I also
> >>>>> noticed that the _Array(size_t) constructor is never used.
> >>>>>
> >>>>>     * include/bits/valarray_array.h (__valarray_get_memory): Remove.
> >>>>>     (__valarray_get_storage): Call operator new directly. Remove
> > ignored
> >>>>>     top-level restrict qualifier and add malloc attribute instead.
> >>>>
> >>>> I am trying to understand the point of adding this attribute. The
> > function
> >>>> is just
> >>>>
> >>>> { return static_cast<_Tp*>(operator new(__n * sizeof(_Tp))); }
> >>>>
> >>>> The idea is that it isn't safe (? see PR 23383) to mark operator new
> > with
> >>>> the attribute, but it is safe for this particular use?
> >>>
> >>> I'd forgotten about that (I was assuming the compiler doesn't need to
> >>> be told about the properties of operator new, because they're defined
> >>> by the language). We can remove the attribute.
> >
> >> I am not necessarily asking to remove it. I don't have a good
> >> understanding of what would break if we marked operator new with the
> >> attribute, so I have no idea if those reasons also apply for this use
in
> >> valarray.
> >
> >>>> When optimizing, I certainly hope this trivial function gets inlined,
> > and
> >>>> then the attribute is lost (should the inliner add 'restrict' when
> > inlining
> >>>> a function with attribute malloc?) and all that matters is operator
> > new.
> >
> >> If we determine that using the attribute here but not on operator new
is
> >> the right choice, then I believe we need some middle-end tweaks so it
> >> isn't ignored.
> >
> > We don't have a good way to do this.  Your suggestion of adding restrict
> > would work if it were not that only function-scope restrict uses are
later
> > handled...

> This seems extremely similar to the issue of inlining functions with
> restrict arguments.

That works to the extent that the effect of restrict is reflected in
the memory references in the IL by PTA.  But for our case there
is no restrict qualified arguments but only return values.

Richard.

> I have written a PR, but it is probably not worth submitting.

> --
> Marc Glisse