From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 71096 invoked by alias); 28 Apr 2015 13:50:20 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 71071 invoked by uid 89); 28 Apr 2015 13:50:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.6 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 28 Apr 2015 13:50:13 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 393548EAF3; Tue, 28 Apr 2015 13:50:11 +0000 (UTC) Received: from [10.10.116.19] ([10.10.116.19]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t3SDoAiP031561; Tue, 28 Apr 2015 09:50:10 -0400 Message-ID: <553F900A.7060601@redhat.com> Date: Tue, 28 Apr 2015 13:57:00 -0000 From: Jason Merrill User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Kai Tietz CC: gcc-patches List Subject: Re: C++ delayed folding branch review References: <5539C519.8070004@redhat.com> <553A8A8E.8070403@redhat.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2015-04/txt/msg01744.txt.bz2 On 04/28/2015 08:06 AM, Kai Tietz wrote: > 2015-04-24 20:25 GMT+02:00 Jason Merrill : >> So for warning folding, I think >> the caching approach we were talking about in the call today is the way to >> go. > > Ok. Just a question about where to hook up the hash-variable. What > would be a good place to create (well this we could do lazy too) it, > and of more importance where to destroy the hash? We could destroy it > after genericize-pass? The important thing is to avoid keeping exprs in the hash table that have already been ggc_collected. So we want to destroy the hash table at least after every top-level declaration, but destroying it more frequently would be fine too if memory bloat seems like an issue. >> Certainly a constant value from maybe_constant_value should not have a >> useless type conversion wrapped around it, as that makes it appear >> non-constant. > > Well, beside an overflow was seen. Right. > The interesting point is here to > find the proper place to do for constructor-elements proper folding, > or at least constant-value folding. cxx_eval_bare_aggregate should already be doing constant-value folding of constructor elements. >> I think that's exactly what I was saying above: "we can't just use >> maybe_constant_value because that only folds C++ constant-expressions, and >> we want to fold more things than that." > > maybe_constant_value folds constants, too. That was actually the > reason to use it. Nevertheless I admit that we could call instead a > ..fully_fold here too. Call it where? > One point I see here about creating a function using > maybe_constant_value + cp_fold is that maybe_constant_value is > something we can call safely in all cases. Right, we would still want the current maybe_constant_value for the few situations where constant expressions affect language semantics but are not required, such as initializers and array bounds. I'm suggesting that we should add maybe_constant_value to cp_fold, not the other way around. >> My point is that cp_fold should be a superset of maybe_constant_value, to >> fix bugs like 53792. And the easiest way to get that would seem to be by >> calling maybe_constant_value from cp_fold. > > Agreed, that bugs like PR 53792 could be solved that way. > Nevertheless they aren't directly linked to the delayed-folding > problem, are they? We could deal with those cases also in > genericize-pass lately, as we want to catch here just a missed > optimization, aren't we? They aren't due to premature folding, but they are due to the lack of delayed folding. If we're going to fully fold expressions at genericize time (and at other times for warnings), that needs to include folding calls to constexpr functions, or it isn't "full" folding. We want to handle them together rather than separately because they can interact: simplifying one expression with fold may turn it into a C++ constant-expression (which is why we have premature-folding bugs), and simplifying a constant-expression may expose more opportunities for fold. And we only want a single hash table. >> OK, that makes sense. Say, a function called like "fold" that only folds >> conversions (and NEGATE_EXPR) of constants. It might make sense to do that >> and otherwise continue to delay folding of conversions. In that context I >> guess this change makes sense. > > Fine, any preferences about place for this function? I suggest the > name 'fold_cst' for it. Sounds good. >> Yes, but we already called maybe_constant_value [in compute_array_index_type]. Calling it again shouldn't >> make any difference. > > We just call it in non-dependent tree, If the expression is dependent, maybe_constant_value does nothing. > and even here just for special cases. Special cases? It looks like it's called if the expression is not a magic C++98 NOP_EXPR with TREE_SIDE_EFFECTS and it is convertible to integral or enumeration type. That seems to cover all cases of constant array bounds. Do you have a testcase that motivates this? >>>>> - itype = fold (itype); >>>>> + itype = maybe_constant_value (itype); >>>>> - itype = variable_size (fold (newitype)); >>>>> + itype = variable_size (maybe_constant_value (newitype)); >>>> >>>> Maybe these should use cp_fully_fold? >>> >>> We could use fully_fold, but we would also modify none-constant >>> expressions by this. Do we actually want that here? >> >> For the first one, I actually think a plain fold is enough, or perhaps >> change the cp_build_binary_op to size_binop. > > Hmm, I doubt that a simple fold would be enough here. As itype is > calculated as minus of two converts (and cp_converts aren't > automatically folded). Sure, the converts need to be folded as well, but we were just saying that we are going to automatically fold conversion of a constant, right? >>>>> @@ -13090,6 +13092,8 @@ build_enumerator (tree name, tree value, tree >>>>> enumtype, location_t loc) >>>>> + if (value) >>>>> + value = maybe_constant_value (value); >> As above, calling the constexpr code twice shouldn't make a difference. > > Well, issue is that cxx_constant_value is just called for > !processing_template_decl case. Is that important? Is there a testcase? Perhaps some change is called for, but adding a redundant call to the constexpr code is not that change. > Additionally we try to > perform_implicit_conversion_flags on it, which can add new > cast-operation, which gets resolved by call for cxx_constant_value. That's after your call to maybe_constant_value, so I don't see how it makes any difference. >> How does delayed folding result in a different OMP_FOR_KIND? > > Well, I guess it is related to optimizations/normalization done early > on parsing-state, which isn't triggered anymore on later folds. As > OMP-stuff isn't that good reading, it is hard to tell why > normalization doesn't happen here anymore. I will try to look into > that, but not sure if it is worth the effort at the very end ... Hmm, sounds like it could be a significant missed optimization. Jason