From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x829.google.com (mail-qt1-x829.google.com [IPv6:2607:f8b0:4864:20::829]) by sourceware.org (Postfix) with ESMTPS id E1D57385743B for ; Tue, 27 Jul 2021 18:56:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E1D57385743B Received: by mail-qt1-x829.google.com with SMTP id x9so10239384qtw.13 for ; Tue, 27 Jul 2021 11:56:12 -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:from:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=HjHIxS3TDc6KqYbNQcKHS02Frlyr92r8kPxN8R+MYXg=; b=rGPGqXPdFWT7KCGgtAH+9Z3dXd9OAcUbbx7wfw1WfqG5hkH6b4+WI59qSrCqY+Cf7K 1q2uVGkKSwq0zIvg4w0WSSsnJU2mbXeDxjD2YCoCqVPcFfFsdZPqjLwqhfHJd8zAeExZ YruF/hyaNb8wax+YpNzd9UXNZSKyilxIB42dj0eI4T9+LynCHs4CdrrHPANpwnlnxImZ pliSwLbAHBBoiRIGlg/Ncn6tua7r5NmCfSKRGLqKjldLom8zyBkQyh5dX/FaZWg7PKkJ uH/GB54DJTZLXDDnsS24j0Fs0CeXYqWe5ezz/DwPHA1IkSrOEDNeBUr5nH+LENOCmje2 gpFQ== X-Gm-Message-State: AOAM532gZBoiQPBMqzME+sHJK7l9sVIXYUNPrwTntgbj3Vw5932PdxGA xBbOLm+exwnCFyCBrwUznm8iTn+xZh8= X-Google-Smtp-Source: ABdhPJwt5dYTYduYaKSHp2PeB9UTNoT6iDBoBHHw25no0OwtaucYZ3fL/F9v29xqWjBn9U/QHDnXkQ== X-Received: by 2002:ac8:514d:: with SMTP id h13mr20737501qtn.176.1627412172385; Tue, 27 Jul 2021 11:56:12 -0700 (PDT) Received: from [192.168.0.41] (75-166-102-22.hlrn.qwest.net. [75.166.102.22]) by smtp.gmail.com with ESMTPSA id t10sm1842947qti.94.2021.07.27.11.56.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 27 Jul 2021 11:56:11 -0700 (PDT) Subject: Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904) From: Martin Sebor To: Jason Merrill , 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> Message-ID: <0bdbdf77-e887-f15a-4b32-0eee6bcf30a9@gmail.com> Date: Tue, 27 Jul 2021 12:56:10 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <6ed67ff9-12d3-516a-f7ec-13dd913ed54b@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, 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: Tue, 27 Jul 2021 18:56:14 -0000 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? 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