From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by sourceware.org (Postfix) with ESMTP id 301653861854 for ; Thu, 6 Aug 2020 19:24:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 301653861854 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-378-if6-AQCEMcGYFZPo2ia84g-1; Thu, 06 Aug 2020 15:24:13 -0400 X-MC-Unique: if6-AQCEMcGYFZPo2ia84g-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B59991DE6; Thu, 6 Aug 2020 19:24:11 +0000 (UTC) Received: from localhost (unknown [10.33.36.145]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4AADA5F21A; Thu, 6 Aug 2020 19:24:11 +0000 (UTC) Date: Thu, 6 Aug 2020 20:24:10 +0100 From: Jonathan Wakely To: Aldy Hernandez Cc: Richard Biener , Andrew MacLeod via Gcc-patches , Martin Jambor , Jeff Law , Andrew MacLeod , Richard Sandiford Subject: Re: std:vec for classes with constructor? Message-ID: <20200806192410.GI3400@redhat.com> References: <5EE6522C-086B-42E0-B42A-F291E09C422A@gmail.com> <83ac1879-ce77-004c-3934-dddd7b9e22c6@redhat.com> <20200806101917.GU3400@redhat.com> <20200806104835.GX3400@redhat.com> <20200806163045.GE3400@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Disposition: inline X-Spam-Status: No, score=-7.7 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Aug 2020 19:24:18 -0000 On 06/08/20 19:58 +0200, Aldy Hernandez wrote: > > >On 8/6/20 6:30 PM, Jonathan Wakely wrote: >>On 06/08/20 16:17 +0200, Aldy Hernandez wrote: >>> >>> >>>On 8/6/20 12:48 PM, Jonathan Wakely wrote: >>>>On 06/08/20 12:31 +0200, Richard Biener wrote: >>>>>On Thu, Aug 6, 2020 at 12:19 PM Jonathan Wakely >>>>> wrote: >>>>>> >>>>>>On 06/08/20 06:16 +0100, Richard Sandiford wrote: >>>>>>>Andrew MacLeod via Gcc-patches writes: >>>>>>>>On 8/5/20 12:54 PM, Richard Biener via Gcc-patches wrote: >>>>>>>>>On August 5, 2020 5:09:19 PM GMT+02:00, Martin Jambor >>>>>> wrote: >>>>>>>>>>On Fri, Jul 31 2020, Aldy Hernandez via Gcc-patches wrote: >>>>>>>>>>[...] >>>>>>>>>> >>>>>>>>>>>* ipa-cp changes from vec to std::vec. >>>>>>>>>>> >>>>>>>>>>>We are using std::vec to ensure constructors are run, which they >>>>>>>>>>aren't >>>>>>>>>>>in our internal vec<> implementation.  Although we >>>>>>>>>>>usually >>>>>>steer away >>>>>>>>>>>from using std::vec because of interactions with our GC system, >>>>>>>>>>>ipcp_param_lattices is only live within the pass >>>>>>>>>>>and >>>>>>allocated with >>>>>>>>>>calloc. >>>>>>>>>>Ummm... I did not object but I will save the URL of >>>>>>>>>>this >>>>>>message in the >>>>>>>>>>archive so that I can waive it in front of anyone >>>>>>>>>>complaining why I >>>>>>>>>>don't use our internal vec's in IPA data structures. >>>>>>>>>> >>>>>>>>>>But it actually raises a broader question: was this >>>>>>supposed to be an >>>>>>>>>>exception, allowed only not to complicate the irange >>>>>>>>>>patch >>>>>>further, or >>>>>>>>>>will this be generally accepted thing to do when >>>>>>>>>>someone >>>>>>wants to have >>>>>>>>>>a >>>>>>>>>>vector of constructed items? >>>>>>>>>It's definitely not what we want. You have to find >>>>>>>>>another >>>>>>solution to this problem. >>>>>>>>> >>>>>>>>>Richard. >>>>>>>>> >>>>>>>> >>>>>>>>Why isn't it what we want? >>>>>>>> >>>>>>>>This is a small vector local to the pass so it doesn't >>>>>>>>interfere with >>>>>>>>our PITA GTY. >>>>>>>>The class is pretty straightforward, but we do need a constructor to >>>>>>>>initialize the pointer and the max-size field.  There is >>>>>>>>no >>>>>>allocation >>>>>>>>done per element, so a small number of elements have a >>>>>>>>couple >>>>>>of fields >>>>>>>>initialized per element. We'd have to loop to do that anyway. >>>>>>>> >>>>>>>>GCC's vec<> does not provide he ability to run a >>>>>>>>constructor, >>>>>>std::vec >>>>>>>>does. >>>>>>> >>>>>>>I realise you weren't claiming otherwise, but: that could >>>>>>>be fixed :-) >>>>>> >>>>>>It really should be. >>>>>> >>>>>>Artificial limitations like that are just a booby trap for the unwary. >>>>> >>>>>It's probably also historic because we couldn't even implement >>>>>the case of re-allocation correctly without std::move, could we? >>>> >>>>I don't see why not. std::vector worked fine without std::move, it's >>>>just more efficient with std::move, and can be used with a wider set >>>>of element types. >>>> >>>>When reallocating you can just copy each element to the new storage >>>>and destroy the old element. If your type is non-copyable then you >>>>need std::move, but I don't think the types I see used with vec<> are >>>>non-copyable. Most of them are trivially-copyable. >>>> >>>>I think the benefit of std::move to GCC is likely to be permitting >>>>cheap copies to be made where previously they were banned for >>>>performance reasons, but not because those copies were impossible. >>> >>>For the record, neither value_range nor int_range require any >>>allocations.  The sub-range storage resides in the stack or >>>wherever it was defined.  However, it is definitely not a POD. >>> >>>Digging deeper, I notice that the original issue that caused us to >>>use std::vector was not in-place new but the safe_grow_cleared.  >>>The original code had: >>> >>>>    auto_vec known_value_ranges; >>>>... >>>>... >>>>    if (!vr.undefined_p () && !vr.varying_p ()) >>>>         { >>>>           if (!known_value_ranges.length ()) >>>>             known_value_ranges.safe_grow_cleared (count); >>>>             known_value_ranges[i] = vr; >>>>         } >>> >>>I would've gladly kept the auto_vec, had I been able to do call >>>the constructor by doing an in-place new: >>> >>>>                   if (!vr.undefined_p () && !vr.varying_p ()) >>>>                     { >>>>                       if (!known_value_ranges.length ()) >>>>-                         known_value_ranges.safe_grow_cleared (count); >>>>+                         { >>>>+                           known_value_ranges.safe_grow_cleared >>>>(count); >>>>+                           for (int i = 0; i < count; ++i) >>>>+                             new (&known_value_ranges[i]) >>>>value_range (); >>>>+                         } >>>>                       known_value_ranges[i] = vr; >>>>                     } >>>>                 } >>> >>>But alas, compiling yields: >>> >>>>In file included from /usr/include/wchar.h:35, >>>>                from /usr/include/c++/10/cwchar:44, >>>>                from /usr/include/c++/10/bits/postypes.h:40, >>>>                from /usr/include/c++/10/iosfwd:40, >>>>                from /usr/include/gmp-x86_64.h:34, >>>>                from /usr/include/gmp.h:59, >>>>                from /home/aldyh/src/gcc/gcc/system.h:687, >>>>                from /home/aldyh/src/gcc/gcc/ipa-fnsummary.c:55: >>>>/home/aldyh/src/gcc/gcc/vec.h: In instantiation of ‘static >>>>size_t vec::embedded_size(unsigned int) [with T >>>>= int_range<1>; A = va_heap; size_t = long unsigned int]’: >>>>/home/aldyh/src/gcc/gcc/vec.h:288:58:   required from ‘static >>>>void va_heap::reserve(vec*&, unsigned int, >>>>bool) [with T = int_range<1>]’ >>>>/home/aldyh/src/gcc/gcc/vec.h:1746:20:   required from ‘bool >>>>vec::reserve(unsigned int, bool) [with T = int_range<1>]’ >>>>/home/aldyh/src/gcc/gcc/vec.h:1766:10:   required from ‘bool >>>>vec::reserve_exact(unsigned int) [with T = int_range<1>]’ >>>>/home/aldyh/src/gcc/gcc/vec.h:1894:3:   required from ‘void >>>>vec::safe_grow(unsigned int) [with T = int_range<1>]’ >>>>/home/aldyh/src/gcc/gcc/vec.h:1912:3:   required from ‘void >>>>vec::safe_grow_cleared(unsigned int) [with T = int_range<1>]’ >>>>/home/aldyh/src/gcc/gcc/ipa-fnsummary.c:634:51:   required from here >>>>/home/aldyh/src/gcc/gcc/vec.h:1285:20: warning: ‘offsetof’ >>>>within non-standard-layout type ‘vec_embedded’ {aka >>>>‘vec, va_heap, vl_embed>’} is >>>>conditionally-supported [-Winvalid-offsetof] >>>>1285 |   return offsetof (vec_embedded, m_vecdata) + alloc * sizeof (T); >>>>     |                    ^ >> >>Can we just avoid using offsetof there? >> >>Untested ... >> >>--- a/gcc/vec.h >>+++ b/gcc/vec.h >>@@ -1281,8 +1281,8 @@ template >>  inline size_t >>  vec::embedded_size (unsigned alloc) >>  { >>-  typedef vec vec_embedded; >>-  return offsetof (vec_embedded, m_vecdata) + alloc * sizeof (T); >>+  size_t offset = (char*)&m_vecdata - (char*)this; >>+  return offset + alloc * sizeof (T); >>  } >> >> > >Now we have: > >In file included from /home/aldyh/src/gcc/gcc/rtl.h:30, > from /home/aldyh/src/gcc/gcc/genpreds.c:27: >/home/aldyh/src/gcc/gcc/vec.h: In static member function ‘static >size_t vec::embedded_size(unsigned int)’: >/home/aldyh/src/gcc/gcc/vec.h:1284:27: error: invalid use of member >‘vec::m_vecdata’ in static member function > 1284 | size_t offset = (char*)&m_vecdata - (char*)this; > | ^~~~~~~~~ >/home/aldyh/src/gcc/gcc/vec.h:626:5: note: declared here > 626 | T m_vecdata[1]; > | ^~~~~~~~~ >/home/aldyh/src/gcc/gcc/vec.h:1284:46: error: ‘this’ is unavailable >for static member functions > 1284 | size_t offset = (char*)&m_vecdata - (char*)this; > | ^~~~ > > Oh sorry, I didn't realise it was static.