From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 5B9C138515F0 for ; Fri, 30 Jul 2021 15:06:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5B9C138515F0 Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-505-eK1Yqo5tOOGk1jm_V69IdA-1; Fri, 30 Jul 2021 11:06:54 -0400 X-MC-Unique: eK1Yqo5tOOGk1jm_V69IdA-1 Received: by mail-qk1-f199.google.com with SMTP id b9-20020a05620a1269b02903b8bd5c7d95so5894701qkl.12 for ; Fri, 30 Jul 2021 08:06:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=V5aSX/RnjEvy3cm1j8Ym6pWY6xbZD7vBB1+zLj/gLog=; b=Y4iEw36ZhzFkBR3T5r8fixkOySpOpc++7IfAFjEm2cAkjoq9SaGxkzEV4kCbhm6ZCm Uvj45k5bGDV6/6u4hAIfSEx4imqb/fbzicOfYM89ohoO2wU5RTDLPN7MEB8dNaPVX486 798VGlKET0CCJSp5Ayfg24JY+Nw3h0M7p+3iG2oKuRsII+US6F8Et7+WEwynhHjin92x VlLAsBvifTbkS7jkyhdl2cLAJ+mYN+nK4GgdpgGZN/EsZ12NWdGWrZRit5yH85WPDZte 08VgqJbnSd4kKCvsWEU7Lpc+VygN2r++7iZxQDDcMsk/8LDqmogqBGiXrmA5rOkiTXHS uq5g== X-Gm-Message-State: AOAM530oM5gKOplXFhPwd2td+oDS8QwP5mDN2DtMGfU193NL/V4jMh2m 0lP2gezwIAH2V9VO2gqpgcXdzqCXg8WpFDTZ51YmrtrtchKljAGkJdFRWjb9IBa6I+ZGMkvJ2fn Z7F3yMNmhVI/ZrbcqiRPL+sqdXaNA6MEKjB1a5fC1hS7zkDf0e09SxQMTmoP7sQT+jw== X-Received: by 2002:a05:620a:120e:: with SMTP id u14mr2708360qkj.309.1627657613196; Fri, 30 Jul 2021 08:06:53 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyVrBQuRONW6u2Q28i299Zfrhke27hEjkEcEGSGYzNkcAW5G3UZuGog3eIwOqTR4dmg/W46Ug== X-Received: by 2002:a05:620a:120e:: with SMTP id u14mr2708327qkj.309.1627657612895; Fri, 30 Jul 2021 08:06:52 -0700 (PDT) Received: from [192.168.1.148] (130-44-159-43.s11817.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id q4sm696499qtw.7.2021.07.30.08.06.52 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 30 Jul 2021 08:06:52 -0700 (PDT) Subject: Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904) To: Martin Sebor , Jonathan Wakely , Richard Biener Cc: gcc-patches References: <390c6652-0a1f-e8c4-d70d-56ced2f7b0fb@gmail.com> <0ee2488c-04a1-df3b-dd4f-92eec51a4ab2@redhat.com> <7ebb37f3-76c3-8e00-7852-c93bf142043a@gmail.com> <2c60a7d0-3f60-b72f-c0f2-6fc7a4900740@redhat.com> <072a4715-2248-d836-948d-50426160de47@gmail.com> <6ed67ff9-12d3-516a-f7ec-13dd913ed54b@gmail.com> <0bdbdf77-e887-f15a-4b32-0eee6bcf30a9@gmail.com> From: Jason Merrill Message-ID: <3c6631e9-d95c-dbf6-e50e-cee99f8652d7@redhat.com> Date: Fri, 30 Jul 2021 11:06:51 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <0bdbdf77-e887-f15a-4b32-0eee6bcf30a9@gmail.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-5.9 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Fri, 30 Jul 2021 15:06:57 -0000 On 7/27/21 2:56 PM, Martin Sebor wrote: > Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575690.html > > Are there any other suggestions or comments or is the latest revision > okay to commit? OK. > On 7/20/21 12:34 PM, Martin Sebor wrote: >> On 7/14/21 10:23 AM, Jason Merrill wrote: >>> On 7/14/21 10:46 AM, Martin Sebor wrote: >>>> On 7/13/21 9:39 PM, Jason Merrill wrote: >>>>> On 7/13/21 4:02 PM, Martin Sebor wrote: >>>>>> On 7/13/21 12:37 PM, Jason Merrill wrote: >>>>>>> On 7/13/21 10:08 AM, Jonathan Wakely wrote: >>>>>>>> On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote: >>>>>>>>> Somebody with more C++ knowledge than me needs to approve the >>>>>>>>> vec.h changes - I don't feel competent to assess all effects of >>>>>>>>> the change. >>>>>>>> >>>>>>>> They look OK to me except for: >>>>>>>> >>>>>>>> -extern vnull vNULL; >>>>>>>> +static constexpr vnull vNULL{ }; >>>>>>>> >>>>>>>> Making vNULL have static linkage can make it an ODR violation to >>>>>>>> use >>>>>>>> vNULL in templates and inline functions, because different >>>>>>>> instantiations will refer to a different "vNULL" in each >>>>>>>> translation >>>>>>>> unit. >>>>>>> >>>>>>> The ODR says this is OK because it's a literal constant with the >>>>>>> same value (6.2/12.2.1). >>>>>>> >>>>>>> But it would be better without the explicit 'static'; then in >>>>>>> C++17 it's implicitly inline instead of static. >>>>>> >>>>>> I'll remove the static. >>>>>> >>>>>>> >>>>>>> But then, do we really want to keep vNULL at all?  It's a weird >>>>>>> blurring of the object/pointer boundary that is also dependent on >>>>>>> vec being a thin wrapper around a pointer.  In almost all cases >>>>>>> it can be replaced with {}; one exception is == comparison, where >>>>>>> it seems to be testing that the embedded pointer is null, which >>>>>>> is a weird thing to want to test. >>>>>> >>>>>> The one use case I know of for vNULL where I can't think of >>>>>> an equally good substitute is in passing a vec as an argument by >>>>>> value.  The only way to do that that I can think of is to name >>>>>> the full vec type (i.e., the specialization) which is more typing >>>>>> and less generic than vNULL.  I don't use vNULL myself so I wouldn't >>>>>> miss this trick if it were to be removed but others might feel >>>>>> differently. >>>>> >>>>> In C++11, it can be replaced by {} in that context as well. >>>> >>>> Cool.  I thought I'd tried { } here but I guess not. >>>> >>>>> >>>>>> If not, I'm all for getting rid of vNULL but with over 350 uses >>>>>> of it left, unless there's some clever trick to make the removal >>>>>> (mostly) effortless and seamless, I'd much rather do it independently >>>>>> of this initial change. I also don't know if I can commit to making >>>>>> all this cleanup. >>>>> >>>>> I already have a patch to replace all but one use of vNULL, but >>>>> I'll hold off with it until after your patch. >>>> >>>> So what's the next step?  The patch only removes a few uses of vNULL >>>> but doesn't add any.  Is it good to go as is (without the static and >>>> with the additional const changes Richard suggested)?  This patch is >>>> attached to my reply to Richard: >>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575199.html >>> >>> As Richard wrote: >>> >>>> The pieces where you change vec<> passing to const vec<>& and the few >>>> where you change vec<> * to const vec<> * are OK - this should make the >>>> rest a smaller piece to review. >>> >>> Please go ahead and apply those changes and send a new patch with the >>> remainder of the changes. >> >> I have just pushed r12-2418: >> https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350886.html >> >>> >>> A few other comments: >>> >>>> -                       omp_declare_simd_clauses); >>>> +                       *omp_declare_simd_clauses); >>> >>> Instead of doing this indirection in all of the callers, let's change >>> c_finish_omp_declare_simd to take a pointer as well, and do the >>> indirection in initializing a reference variable at the top of the >>> function. >> >> Okay. >> >>> >>>> +    sched_init_luids (bbs.to_vec ()); >>>> +    haifa_init_h_i_d (bbs.to_vec ()); >>> >>> Why are these to_vec changes needed when you are also changing the >>> functions to take const&? >> >> Calling to_vec() here isn't necessary so I've removed it. >> >>> >>>> -  vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo); >>>> +  vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo).to_vec (); >>> >>> Why not use a reference here and in other similar spots? >> >> Sure, that works too. >> >> Attached is what's left of the original changes now that r12-2418 >> has been applied. >> >> Martin >