From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by sourceware.org (Postfix) with ESMTP id 628C03858D35 for ; Thu, 30 Jul 2020 14:57:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 628C03858D35 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-34-BGiGjPx-PM-UBx-2BQDjGg-1; Thu, 30 Jul 2020 10:57:37 -0400 X-MC-Unique: BGiGjPx-PM-UBx-2BQDjGg-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 3EA498015CE for ; Thu, 30 Jul 2020 14:57:36 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-112-218.ams2.redhat.com [10.36.112.218]) by smtp.corp.redhat.com (Postfix) with ESMTPS id CD6E610013C4; Thu, 30 Jul 2020 14:57:35 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id 06UEvWHF014118; Thu, 30 Jul 2020 16:57:32 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id 06UEvWci014117; Thu, 30 Jul 2020 16:57:32 +0200 Date: Thu, 30 Jul 2020 16:57:32 +0200 From: Jakub Jelinek To: Jason Merrill Cc: gcc-patches@gcc.gnu.org, Jonathan Wakely Subject: Re: [PATCH] c++: Add __builtin_bit_cast to implement std::bit_cast [PR93121] Message-ID: <20200730145732.GZ2363@tucnak> Reply-To: Jakub Jelinek References: <20200718185056.GN2363@tucnak> <0dda7918-cc7f-3f3f-64c0-34896ef9e15d@redhat.com> MIME-Version: 1.0 In-Reply-To: <0dda7918-cc7f-3f3f-64c0-34896ef9e15d@redhat.com> User-Agent: Mutt/1.11.3 (2019-02-01) X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-7.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, 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, 30 Jul 2020 14:57:44 -0000 On Thu, Jul 30, 2020 at 10:16:46AM -0400, Jason Merrill wrote: > > It checks the various std::bit_cast requirements, when not constexpr > > evaluated acts pretty much like VIEW_CONVERT_EXPR of the source argument > > to the destination type and the hardest part is obviously the constexpr > > evaluation. I couldn't use the middle-end native_encode_initializer, > > because it needs to handle the C++ CONSTRUCTOR_NO_CLEARING vs. negation of > > that > > CONSTRUCTOR_NO_CLEARING isn't specific to C++. That is true, but the middle-end doesn't really use that much (except one spot in the gimplifier and when deciding if two constructors are equal). > > value initialization of missing members if there are any etc. > > Doesn't native_encode_initializer handle omitted initializers already? Any > elements for which the usual zero-initialization is wrong should already > have explicit initializers by the time we get here. It assumes zero initialization for everything that is not explicitly initialized. When it is used in the middle-end, CONSTRUCTORs are either used in initializers of non-automatic vars and then they are zero initialized, or for automatic variables only empty CONSTRUCTORs are allowed. So, what it does is memset the memory to zero before trying to fill in the individual initializers for RECORD_TYPE/ARRAY_TYPE. Even if we are guaranteed that (what guarantees it?) when __builtin_bit_cast is constexpr evaluated no initializer will be omitted if may be value initialized to something other than all zeros, we still need to track what bits are well defined and what are not (e.g. any padding in there). Thinking about it now, maybe the mask handling for !CONSTRUCTOR_NO_CLEARING is incorrect though if there are missing initializers, because those omitted initializers still could have paddings with unspecified content, right? > > needs to handle bitfields even if they don't have an integral representative > > Can we use TYPE_MODE for this? When the bitfield representative is an array, it will have BLKmode TYPE_MODE, and we need to find some suitable other integral type. The bit-field handling for BLKmode representatives can be copied to the middle-end implementation. > These all sound like things that could be improved in > native_encode_initializer rather than duplicate a complex function. I think in the end only small parts of it are actually duplicated. It is true that both native_encode_initializer and the new C++ FE counterpart are roughly 310 lines long, but the mask handling the latter performs is unlikely useful for the middle-end (and if added there it would need to allow it thus to be NULL and not filled), on the other side the very complex handling the middle-end version needs to do where it can be asked only for a small portion of the memory complicates things a lot. Maybe we could say that non-NULL mask is only supported for the case where one asks for everything and have some assertions for that, but I fear we'd still end up with ~ 450-500 lines of code in the middle-end compared to the current 310. I guess I can try to merge the two back in one with optional mask and see how does it look like. But most likely the !CONSTRUCTOR_NO_CLEARING handling etc. would need to be conditionalized on this special C++ mode (mask non-NULL) etc. > > + if (REFERENCE_REF_P (arg)) > > + arg = TREE_OPERAND (arg, 0); > > Why? I've added it so that the decay_conversion later on didn't mishandle references to arrays. > > + if (error_operand_p (arg)) > > + return error_mark_node; > > + > > + if (!type_dependent_expression_p (arg)) > > + { > > + if (TREE_CODE (TREE_TYPE (arg)) == ARRAY_TYPE) > > + { > > + /* Don't perform array-to-pointer conversion. */ > > + arg = mark_rvalue_use (arg, loc, true); > > + if (!complete_type_or_maybe_complain (TREE_TYPE (arg), arg, complain)) > > + return error_mark_node; > > + } But then I've added the above to match more the __builtin_bit_cast behavior in clang, so maybe the if (REFERENCE_REF_P (arg)) is not needed anymore. For the std::bit_cast uses, the argument will be always a reference, never an array directly and not sure if we want to encourage people to use the builtin directly in their code. > > + else > > + arg = decay_conversion (arg, complain); > > + > > + if (error_operand_p (arg)) > > + return error_mark_node; > > + > > + arg = convert_from_reference (arg); > > + if (!trivially_copyable_p (TREE_TYPE (arg))) > > + { > > + error_at (cp_expr_loc_or_loc (arg, loc), > > + "%<__builtin_bit_cast%> source type %qT " > > + "is not trivially copyable", TREE_TYPE (arg)); > > + return error_mark_node; > > + } > > + if (!dependent_type_p (type) > > + && !cp_tree_equal (TYPE_SIZE_UNIT (type), > > + TYPE_SIZE_UNIT (TREE_TYPE (arg)))) > > + { > > + error_at (loc, "%<__builtin_bit_cast%> source size %qE " > > + "not equal to destination type size %qE", > > + TYPE_SIZE_UNIT (TREE_TYPE (arg)), > > + TYPE_SIZE_UNIT (type)); > > + return error_mark_node; > > + } > > + } > > + > > + tree ret = build_min (BIT_CAST_EXPR, type, arg); > > + SET_EXPR_LOCATION (ret, loc); > > + return ret; > > +} > > + > > #include "gt-cp-semantics.h" > > --- gcc/cp/tree.c.jj 2020-07-18 10:48:51.738489259 +0200 > > +++ gcc/cp/tree.c 2020-07-18 10:52:46.188021206 +0200 > > @@ -3908,6 +3908,11 @@ cp_tree_equal (tree t1, tree t2) > > return false; > > return true; > > + case BIT_CAST_EXPR: > > + if (!same_type_p (TREE_TYPE (t1), TREE_TYPE (t2))) > > + return false; > > + break; > > Add this to the *_CAST_EXPR cases like you do in cp_walk_subtrees? Ok. > > + { > > + if (type == orig_type) > > + error_at (loc, "%qs is not constant expression because %qT is " > > + "a union type", "__builtin_bit_cast", type); > > "not a constant expression"; you're missing the "a" in all of these. Thanks, will change. Jakub