From patchwork Fri Jul 31 21:54:26 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Kai Tietz X-Patchwork-Id: 502744 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id C5F0A140E3F for ; Sat, 1 Aug 2015 07:54:35 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=iCc39gEE; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:message-id:in-reply-to:references:subject :mime-version:content-type:content-transfer-encoding; q=dns; s= default; b=G6ZvtEOUBkni0URzAB8nWBCc5VRYBkZr4IMP738qukqYLqSWq8erA PXYW3FA1b/4ol4UW49u1bExOdx8MximmMiqx00V6hkQbX5+CJMzMnvSWya45arLM kCvbX0CWPlnafcqBCUJnsDR0VmUpTBMmw0PuBQ3jYpi4LaBMzYZp9s= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:message-id:in-reply-to:references:subject :mime-version:content-type:content-transfer-encoding; s=default; bh=gmQwV3d+qQ/NkxGKFqnKolOjhRc=; b=iCc39gEEn3tNUfJPF16m083v36O4 L5Ssb22gE6m+ikxayhzpZkdKO0gVU7rfLiwQsS6G2wemq4nlrQ+KrTRQf3y0Fi6A xyfnSwi1i9D/PY3kBLGl5egYYuJHwrB3rLwiHbyTp1mcrcRlzU6QLsgUtTrZFrYJ 0IOewpdv7o6J1qo= Received: (qmail 65580 invoked by alias); 31 Jul 2015 21:54:29 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 65569 invoked by uid 89); 31 Jul 2015 21:54:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx5-phx2.redhat.com Received: from mx5-phx2.redhat.com (HELO mx5-phx2.redhat.com) (209.132.183.37) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 31 Jul 2015 21:54:27 +0000 Received: from zmail14.collab.prod.int.phx2.redhat.com (zmail14.collab.prod.int.phx2.redhat.com [10.5.83.16]) by mx5-phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t6VLsQPQ057912; Fri, 31 Jul 2015 17:54:26 -0400 Date: Fri, 31 Jul 2015 17:54:26 -0400 (EDT) From: Kai Tietz To: Jason Merrill Cc: Kai Tietz , gcc-patches List Message-ID: <597173047.4338388.1438379666336.JavaMail.zimbra@redhat.com> In-Reply-To: <55BAACF9.7040707@redhat.com> References: <557A5214.7060106@redhat.com> <55B911DD.30105@redhat.com> <55BA5667.9040200@redhat.com> <55BAACF9.7040707@redhat.com> Subject: Re: C++ delayed folding branch review MIME-Version: 1.0 X-IsSubscribed: yes ----- Ursprüngliche Mail ----- > On 07/30/2015 05:00 PM, Kai Tietz wrote: > > 2015-07-30 18:52 GMT+02:00 Jason Merrill : > >> On 07/29/2015 06:56 PM, Kai Tietz wrote: > >>>>>>>>>>> @@ -13078,6 +13042,8 @@ build_enumerator (tree name, tree value, > >>>>>>>>>>> tree > >>>>>>>>>>> enumtype, tree attributes, > >>>>>>>>>>> if (value) > >>>>>>>>>>> STRIP_TYPE_NOPS (value); > >>>>>>>>>>> > >>>>>>>>>>> + if (value) > >>>>>>>>>>> + value = cp_try_fold_to_constant (value); > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Again, this is unnecessary because we call cxx_constant_value > >>>>>>>>>> below. > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> See nops, and other unary-operations we want to reduce here to real > >>>>>>>>> constant value ... > >>>>>>>> > >>>>>>>> > >>>>>>>> The cxx_constant_value call below will deal with them. > >>>>>>> > >>>>>>> > >>>>>>> Likewise for grokbitfield. > >>>>>> > >>>>>> > >>>>>> Hmm, AFAIR we don't call cxx_constant_value in all code-paths. But I > >>>>>> will look into it, and come back to you on it. > >>>>> > >>>>> > >>>>> I am still on it ... first did the other points > >>>> > >>>> > >>>> Looks like this hasn't changed. > >>> > >>> > >>> Yes, for grokbitfield current version uses fold_simple for witdth. So > >>> just expressions based on constants getting reduced to short form. In > >>> grokbitfield I don't see invocation of cxx_constant_value. So how can > >>> we be sure that width is reduced to integer-cst? > >> > >> > >> We call cxx_constant_value on bit-field widths in check_bitfield_decl. > > > > Hmm, ok. But I don't see that this function gets called in context of > > grokbitfield, after we set DECL_INITIAL. > > Nope, it's called later on as part of finish_struct. > Ok, adjusted. > > By removing this folding here, we get new failures in > > g++.dg/warn/overflow-warn-1.C testcase: > > New errors are at lin 32 that 'bif-foeld 's::' width not an > > integer constant' > > and at same line ''(1 / 0) is not a constant expression'. Those > > message don't look wrong. > > > > The testcase next to this 'overflow-warn-3.C and overflow-warn-4.C' > > failing in the same manner for (1 / 0) case. But there are no other > > regressions in g++.dg & libstdc++ > > > > Shall I extend the testcases for this message? > > Please. Done. > >>>>>>>>>>> @@ -1947,6 +1947,8 @@ build_complex (tree type, tree real, tree > >>>>>>>>>>> imag) > >>>>>>>>>>> { > >>>>>>>>>>> tree t = make_node (COMPLEX_CST); > >>>>>>>>>>> > >>>>>>>>>>> + real = fold (real); > >>>>>>>>>>> + imag = fold (imag); > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> I still think this is wrong. The arguments should be sufficiently > >>>>>>>>>> folded. > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> As we don't fold unary-operators on constants, we need to fold it > >>>>>>>>> at > >>>>>>>>> some place. AFAICS is the C++ FE not calling directly > >>>>>>>>> build_complex. > >>>>>>>>> So this place was the easiest way to avoid issues with things like > >>>>>>>>> '-' > >>>>>>>>> '1' etc. > >>>>>>>> > >>>>>>>> > >>>>>>>> Is this because of the > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> value = build_complex (NULL_TREE, convert (const_type, > >>>>>>>>> > >>>>>>>>> integer_zero_node), > >>>>>>>>> value); > >>>>>> > >>>>>> > >>>>>> Might be. This should be indeed a 'fold_convert', isn't it? > >>>> > >>>> > >>>> Yes. > >>> > >>> > >>> Applied modification to it. > >> > >> > >> So can we remove the fold in build_complex now? > Yes. Done. > > >>>>>>>> in interpret_float? I think "convert" definitely needs to do some > >>>>>>>> folding, since it's called from middle-end code that expects that. > >>>>>>> > >>>>>>> > >>>>>>> I remember talking about "convert" doing some folding (and cp_convert > >>>>>>> not) in our 1:1 last week. > >>>>>> > >>>>>> > >>>>>> Can't remember that. I know that we were talking about the difference > >>>>>> of convert and fold_convert. convert can be used on C++ specifics, > >>>>>> but fold_convert is something shared with ME. > >>>> > >>>> > >>>> convert is called from the ME, which sometimes expects folding. > >>>> > >>>>>> So first 'fold_convert' > >>>>>> isn't the same as 'fold (convert ())'. > >>>>>> I don't find places we invoke convert () in ME. We have some calls in > >>>>>> convert.c (see convert_to_integer, convert_to_integer_nofold, and > >>>>>> convert_to_real), which all used in AST only AFAICS. > >>>> > >>>> > >>>> I was thinking of convert.c and fold-const.c to be part of the ME, since > >>>> they are language-independent. But I guess other people think of the ME > >>>> starting with gimple. > >>>> > >>>> And it looks like the only language-independent uses of convert are in > >>>> c-family; I guess many of them should change to fold_convert. > >>> > >>> > >>> Hmm, in context of this work? Or is this more a general point about > >>> future > >>> work? > >> > >> > >> In the context of this work, if they are introducing problematic NOPs. > > > > Ok, I will take a closer look to convert () usage in c-family/. By > > quick looking this seems to be the only place for now we needed to > > change. > > > >>>>>>>>>>> @@ -5080,6 +5081,7 @@ output_constructor_bitfield (oc_local_state > >>>>>>>>>>> *local, > >>>>>>>>>>> unsigned int bit_offset) > >>>>>>>>>>> while (TREE_CODE (local->val) == VIEW_CONVERT_EXPR > >>>>>>>>>>> || TREE_CODE (local->val) == NON_LVALUE_EXPR) > >>>>>>>>>>> local->val = TREE_OPERAND (local->val, 0); > >>>>>>>>>>> + local->val = fold (local->val); > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Likewise. > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> As soon as we can be sure that values getting fully_folded, or at > >>>>>>>>> least folded for constants, we should be able to remove this. > >>>>>>>> > >>>>>>>> > >>>>>>>> Yep, they need to be folded before we get here. > >>> > >>> > >>> I didn't come to remove this line for testing. As we fold now for > >>> initializers more early, and cp_fold supports constructors, it could > >>> be that we don't need this anymore. It is on my pile. > >> > >> > >>> That fold is still required. By removing it, I saw boostrap issue due > >>> 'invalid initializer'. > >> > >> > >> That indicates a folding problem earlier on, that will cause some > >> initialization that should be performed at compile time to happen at run > >> time instead. > >> > >> Please investigate the bootstrap issue further. > > > > Yes, I do. I assume it is related to 'store_init_value'. For cases > > decl_maybe_constant_var_p() is true, or decl is a static, we are > > calling maybe_constant_init on the value, but for other cases we don't > > simplify value. (Btw this might be related to the > > STRIP_NOPS-requirement in 'reduced_constant_expression_p'). So by > > adding the following hunk it seems to work (still need to verify) > > > > Index: typeck2.c > > =================================================================== > > --- typeck2.c (Revision 226401) > > +++ typeck2.c (Arbeitskopie) > > @@ -833,6 +833,8 @@ store_init_value (tree decl, tree init, vec > DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl) = const_init; > > TREE_CONSTANT (decl) = const_init && decl_maybe_constant_var_p > > (decl); > > } > > + else > > + value = fold_simple (value); > > > > if (cxx_dialect >= cxx14 && CLASS_TYPE_P (strip_array_types (type))) > > /* Handle aggregate NSDMI in non-constant initializers, too. */ > > I guess we want to extend the code for handling statics and constants to > also handle the case where the initializer is a CONSTRUCTOR. And also > fold individual elements in split_nonconstant_init. Yes, I added a general full folding to store_init_value, and to split_nonconstant_init a folding to CONSTRUCTOR handling for RECORD_TYPES, etc. By this I was able to remove the fold from varasm.c's output-function, too. The invocation in split_nonconstant_init might not be necessary, but there are other callers then store_init_value. So for them folding in those cases might be still benifitial. The "STRIP_NOPS-requirement in 'reduced_constant_expression_p'" I could remove, but for one case in constexpr. Without folding we don't do type-sinking/raising. So binary/unary operations might be containing cast, which were in the past unexpected. On verify_constant we check by reduced_constant_expression_p, if value is a constant. We don't handle here, that NOP_EXPRs are something we want to look through here, as it doesn't change anything if this is a constant, or not. So I suggest following patch, so we are able to remove the STRIP_NOPS from reduced_constant_expression_p. Regards, Kai > Jason > > --- constexpr.c (revision 226452) +++ constexpr.c (working copy) @@ -1441,8 +1441,6 @@ cxx_eval_call_expression (const constexpr_ctx *ctx bool reduced_constant_expression_p (tree t) { - /* Make sure we remove useless initial NOP_EXPRs. */ - STRIP_NOPS (t); switch (TREE_CODE (t)) { case PTRMEM_CST: @@ -1476,7 +1474,10 @@ static bool verify_constant (tree t, bool allow_non_constant, bool *non_constant_p, bool *overflow_p) { - if (!*non_constant_p && !reduced_constant_expression_p (t)) + tree rde = t; + + STRIP_NOPS (rde); + if (!*non_constant_p && !reduced_constant_expression_p (rde)) { if (!allow_non_constant) error ("%q+E is not a constant expression", t);