From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 130725 invoked by alias); 28 Apr 2015 12:06:27 -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 130658 invoked by uid 89); 28 Apr 2015 12:06:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wi0-f181.google.com Received: from mail-wi0-f181.google.com (HELO mail-wi0-f181.google.com) (209.85.212.181) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 28 Apr 2015 12:06:25 +0000 Received: by wizk4 with SMTP id k4so137511307wiz.1 for ; Tue, 28 Apr 2015 05:06:21 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.180.86.69 with SMTP id n5mr28741820wiz.91.1430222781912; Tue, 28 Apr 2015 05:06:21 -0700 (PDT) Received: by 10.28.178.149 with HTTP; Tue, 28 Apr 2015 05:06:21 -0700 (PDT) In-Reply-To: <553A8A8E.8070403@redhat.com> References: <5539C519.8070004@redhat.com> <553A8A8E.8070403@redhat.com> Date: Tue, 28 Apr 2015 12:06:00 -0000 Message-ID: Subject: Re: C++ delayed folding branch review From: Kai Tietz To: Jason Merrill Cc: gcc-patches List Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes X-SW-Source: 2015-04/txt/msg01724.txt.bz2 2015-04-24 20:25 GMT+02:00 Jason Merrill : > On 04/24/2015 09:46 AM, Kai Tietz wrote: >> >> Sure, we can use here instead *_fully_fold, but for what costs? In >> general we need to deal here a simple one-level fold for simplifying >> constant-values, and/or removing useless type-conversions. > > > Well, here you're doing a two-level fold. And in general fold relies on > subexpressions being appropriately folded. 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? >>>> @@ -597,9 +597,9 @@ null_member_pointer_value_p (tree t) >>>> return false; >>>> else if (TYPE_PTRMEMFUNC_P (type)) >>>> return (TREE_CODE (t) == CONSTRUCTOR >>>> - && integer_zerop (CONSTRUCTOR_ELT (t, 0)->value)); >>>> + && integer_zerop (fold (CONSTRUCTOR_ELT (t, 0)->value))); >>>> else if (TYPE_PTRDATAMEM_P (type)) >>>> - return integer_all_onesp (t); >>>> + return integer_all_onesp (fold (t)); >>> >>> >>> >>> Calling fold here is wrong; it doesn't handle constexpr, and we should >>> have >>> folded before we got here. >> >> >> s.o. we need to make sure constant-values get rid of useless >> types-conversions/negates/etc ... > > > 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. The interesting point is here to find the proper place to do for constructor-elements proper folding, or at least constant-value folding. I made here already some tries to find a proper place for this, but ran by this into troubles that not in all cases maybe_constant_value was callable (endless recursion). Additionally it happens in some cases for constructor-elements that nop-casts are added on assign. At least I ran into that. In general there is the question if we shouldn't do for constructors call a ...fully_fold? >>>> @@ -576,7 +576,6 @@ build_simple_base_path (tree expr, tree binfo) >>>> expr = build3 (COMPONENT_REF, >>>> cp_build_qualified_type (type, type_quals), >>>> expr, field, NULL_TREE); >>>> - expr = fold_if_not_in_template (expr); >>> >>> >>> I don't think we need to remove this fold, since it is part of compiler >>> internals rather than something the user wrote. Really, we should >>> represent >>> the base conversion with something like a CONVERT_EXPR and only call this >>> function when we want to fold it. But that can wait for a later patch. >> >> >> Ok. I remove this fold-case due simply removing >> fold_if_not_in_template function. So well, we could re-add a call for >> fold, if not in template. > > > Let's try not checking for being in a template, see if it breaks. I tested to use here just a fold (), and I ddn't noticed any fallout by this. So I committed it to branch. >>>> +static tree >>>> +cp_fold (tree x, hash_map *fold_hash) >>>> +{ >>> >>> >>> .... >>> >>> I still think we need a hybrid of this and the constexpr code: it isn't >>> full >>> folding if we aren't doing constexpr evaluation. But we can't just use >>> maybe_constant_value because that only folds C++ constant-expressions, >>> and >>> we want to fold more things than that. I suppose one simple approach for >>> now would be to call maybe_constant_value from cp_fold. >> >> >> Well, the functionality of cp_fold and maybe_constant_value (well, >> actually how constexpr.c works) are different in cases of >> non-constant results. > > > 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. 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. > 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? >>>> @@ -614,9 +614,13 @@ cp_fold_convert (tree type, tree expr) >>>> } >>>> else >>>> { >>>> - conv = fold_convert (type, expr); >>>> + if (TREE_CODE (expr) == INTEGER_CST) >>>> + conv = fold_convert (type, expr); >>>> + else >>>> + conv = convert (type, expr); >>> >>> >>> Why? If we're calling cp_fold_convert in a place where we don't want to >>> fold, we should stop calling it rather than change it. > > >> See, that we want to take care that constant-value is found here. >> Otherwise we don't want anything folded. Well, we could introduce >> for this a special routine to abstract intention here. > > > 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. >>>> @@ -8502,16 +8502,18 @@ compute_array_index_type (tree name, tree size, >>>> tsubst_flags_t complain) >>>> + /* We need to do fully folding to determine if we have VLA, or not. >>>> */ >>>> + tree size_constant = maybe_constant_value (size); >>> >>> >>> Why is this needed? We already call maybe_constant_value earlier in >>> compute_array_index_type. >> >> >> Well, see above. We might have constant-value not simplified. So we >> need a way to make sure we simplify in such case, but if it is >> none-constant, we don't want an modified expression. So >> maybe_constant_value does this ... > > > Yes, but we already called maybe_constant_value. Calling it again shouldn't > make any difference. We just call it in none-dependent tree, and even here just for special cases. We need to make sure to simplifiy to constant. Well, we could use here instead fold_cst instead (which calls internally ...fully_fold, but just returns altered tree if result is constant). >>>> - 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 minux of two converts (and cp_converts aren't automatically folded). > For the second, can we drop that whole block (the one starting with "if > (TREE_CODE (itype) != SAVE_EXPR)") and rely on late folding to handle > SIZEOF_EXPR? I am testing >>>> @@ -13090,6 +13092,8 @@ build_enumerator (tree name, tree value, tree >>>> enumtype, location_t loc) >>>> + if (value) >>>> + value = maybe_constant_value (value); >>> >>> >>> >>> This seems unnecessary, since we call cxx_constant_value below. >> >> >> See the other places > > > 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. Additionally we try to perform_implicit_conversion_flags on it, which can add new cast-operation, which gets resulved by call for cxx_constant_value. So it makes a difference AFAIC, but I might misread here > > 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 ... > > Jason > Kai