From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22966 invoked by alias); 30 Jul 2015 13:50:03 -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 22957 invoked by uid 89); 30 Jul 2015 13:50:02 -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; Thu, 30 Jul 2015 13:49:56 +0000 Received: by wibxm9 with SMTP id xm9so69731579wib.1 for ; Thu, 30 Jul 2015 06:49:53 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.194.201.71 with SMTP id jy7mr91790311wjc.93.1438264193372; Thu, 30 Jul 2015 06:49:53 -0700 (PDT) Received: by 10.27.176.72 with HTTP; Thu, 30 Jul 2015 06:49:53 -0700 (PDT) In-Reply-To: References: <557A5214.7060106@redhat.com> <1424811417.1214725.1434125493982.JavaMail.zimbra@redhat.com> <557BAE5A.7030309@redhat.com> <55B661A1.6090308@redhat.com> <55B911DD.30105@redhat.com> Date: Thu, 30 Jul 2015 14:40:00 -0000 Message-ID: Subject: Re: C++ delayed folding branch review From: Kai Tietz To: Jason Merrill Cc: Kai Tietz , gcc-patches List Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg02571.txt.bz2 2015-07-30 0:56 GMT+02:00 Kai Tietz : > 2015-07-29 19:48 GMT+02:00 Jason Merrill : >> On 07/28/2015 04:10 PM, Kai Tietz wrote: > The change to adjust_temp_type seems to be no more necessary (just > doing tests on it). Yes, committed it. >> >>>>>>>>> @@ -3391,8 +3431,23 @@ cxx_eval_constant_expression (const >>>>>>>>> constexpr_ctx >>>>>>>>> *ctx, tree t, >>>>>>>>> case CONVERT_EXPR: >>>>>>>>> case VIEW_CONVERT_EXPR: >>>>>>>>> case NOP_EXPR: >>>>>>>>> + case UNARY_PLUS_EXPR: >>>>>>>>> { >>>>>>>>> + enum tree_code tcode = TREE_CODE (t); >>>>>>>>> tree oldop = TREE_OPERAND (t, 0); >>>>>>>>> + >>>>>>>>> + if (tcode == NOP_EXPR && TREE_TYPE (t) == TREE_TYPE (oldop) >>>>>>>>> && >>>>>>>>> TREE_OVERFLOW_P (oldop)) >>>>>>>>> + { >>>>>>>>> + if (!ctx->quiet) >>>>>>>>> + permerror (input_location, "overflow in constant >>>>>>>>> expression"); >>>>>>>>> + /* If we're being permissive (and are in an enforcing >>>>>>>>> + context), ignore the overflow. */ >>>>>>>>> + if (!flag_permissive) >>>>>>>>> + *overflow_p = true; >>>>>>>>> + *non_constant_p = true; >>>>>>>>> + >>>>>>>>> + return t; >>>>>>>>> + } >>>>>>>>> tree op = cxx_eval_constant_expression (ctx, oldop, >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Why doesn't the call to cxx_eval_constant_expression at the bottom >>>>>>>> here >>>>>>>> handle oldop having TREE_OVERFLOW set? >>>>>>> >>>>>>> >>>>>>> >>>>>>> I just handled the case that we see here a wrapping NOP_EXPR around an >>>>>>> overflow. As this isn't handled by cxx_eval_constant_expression. >>>>>> >>>>>> >>>>>> >>>>>> How does it need to be handled? A NOP_EXPR wrapped around an overflow >>>>>> is there to indicated that the expression is non-constant, and it can't >>>>>> be simplified any farther. >>>>>> >>>>>> Please give an example of what was going wrong. >> >> ^ > > I did some regression-testing on it. This looks to me like something > I missed to cleanup. Most changes within constexpr-code aren't > necessary anymore. But looking on that, I think I papered over some > issues I had about double-reporting of non-constant expression on > overflows. Committed change to branch for removing this. >>>>>>>>> @@ -565,6 +571,23 @@ cp_gimplify_expr (tree *expr_p, gimple_seq >>>>>>>>> *pre_p, >>>>>>>>> gimple_seq *post_p) >>>>>>>>> >>>>>>>>> switch (code) >>>>>>>>> { >>>>>>>>> + case SIZEOF_EXPR: >>>>>>>>> + if (SIZEOF_EXPR_TYPE_P (*expr_p)) >>>>>>>>> + *expr_p = cxx_sizeof_or_alignof_type (TREE_TYPE >>>>>>>>> (TREE_OPERAND >>>>>>>>> (*expr_p, >>>>>>>>> + >>>>>>>>> 0)), >>>>>>>>> + SIZEOF_EXPR, false); >>>>>>>>> + else if (TYPE_P (TREE_OPERAND (*expr_p, 0))) >>>>>>>>> + *expr_p = cxx_sizeof_or_alignof_type (TREE_OPERAND (*expr_p, >>>>>>>>> 0), >>>>>>>>> + SIZEOF_EXPR, false); >>>>>>>>> + else >>>>>>>>> + *expr_p = cxx_sizeof_or_alignof_expr (TREE_OPERAND (*expr_p, >>>>>>>>> 0), >>>>>>>>> + SIZEOF_EXPR, false); >>>>>>>>> + if (*expr_p == error_mark_node) >>>>>>>>> + *expr_p = size_one_node; >>>>>>>>> + >>>>>>>>> + *expr_p = maybe_constant_value (*expr_p); >>>>>>>>> + ret = GS_OK; >>>>>>>>> + break; >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Why are these surviving until gimplification time? >>>>>>> >>>>>>> >>>>>>> This might be still necessary. I will retest, when bootstrap works. >>>>>>> As we now added SIZEOF_EXPR folding to cp_fold, and if we catch all >>>>>>> expressions a sizeof can occure, this shouldn't be necessary anymore. >>>>>>> AFAIR I saw here some issues about initialzation for global-variables, >>>>>>> which weren't caught. >>>>>> >>>>>> >>>>>> >>>>>> Hmm, I wonder why you would see issues with global initializers that >>>>>> aren't seen on trunk? In any case, if the issue is with global >>>>>> initializers, they should be handled sooner, not here. >> > > They don't survice in function-context, but outside they might. On > trunk we never will see an sizeof-expression in such case as they got > folded-away much earlier. > > I will try an bootstrap with disabling it. In ME we don't produce > sizeof-expressions anymore, so we don't need to think about > re-gimplifiying some AST AFAICS. Tested. We seems not to need the handle of SIZEOF_EXPR in gimplifier anymore. so removed hunk. >> >>>>>>>>> @@ -1529,8 +1532,11 @@ build_expr_type_conversion (int desires, tree >>>>>>>>> expr, >>>>>>>>> bool complain) >>>>>>>>> tree basetype = TREE_TYPE (expr); >>>>>>>>> tree conv = NULL_TREE; >>>>>>>>> tree winner = NULL_TREE; >>>>>>>>> + /* Want to see if EXPR is a constant. See below checks for >>>>>>>>> null_node. >>>>>>>>> */ >>>>>>>>> + tree expr_folded = cp_try_fold_to_constant (expr); >>>>>>>>> >>>>>>>>> - if (expr == null_node >>>>>>>>> + STRIP_NOPS (expr_folded); >>>>>>>>> + if (expr_folded == null_node >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Again, we shouldn't need to fold to check for null_node, it only >>>>>>>> occurs >>>>>>>> when explicitly written. Folding should never produce null_node >>>>>>>> unless >>>>>>>> the argument was already null_node. >>>>>>> >>>>>>> >>>>>>> >>>>>>> Well, we need to do this for diagnostic messages AFAIR. We want to >>>>>>> see if expression folded gets a constant, so that diagnostics getting >>>>>>> displayed right. >>>>>> >>>>>> >>>>>> >>>>>> Again, null_node is special. It indicates that the user typed >>>>>> "__null". >>>>>> That's what we're checking for here. Folding is both unnecessary and >>>>>> undesirable. >> > > So, let us remove it ... I expect issues about casts on integers, > which are reasoned due implicit assignments, expr won't be null_node. I was wrong about this. I removed the folding from build_expr_type_conversion routine. >> >>>>>>>>> @@ -1548,7 +1554,7 @@ build_expr_type_conversion (int desires, tree >>>>>>>>> expr, >>>>>>>>> bool complain) >>>>>>>>> switch (TREE_CODE (basetype)) >>>>>>>>> { >>>>>>>>> case INTEGER_TYPE: >>>>>>>>> - if ((desires & WANT_NULL) && null_ptr_cst_p (expr)) >>>>>>>>> + if ((desires & WANT_NULL) && null_ptr_cst_p (expr_folded)) >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Again, we don't want to fold before calling null_ptr_cst_p, since in >>>>>>>> C++11 only a literal 0 is a null pointer constant. For C++98 we >>>>>>>> already >>>>>>>> fold in null_ptr_cst_p. >>>>>>> >>>>>>> >>>>>>> >>>>>>> We need to avoid useless conversion, so we should reduce to simple >>>>>>> constant-value ... >>>>>> >>>>>> >>>>>> >>>>>> No. Again, in C++11 only "0" or "0L" is a null pointer constant. A >>>>>> more complex expression that folds to 0 is NOT a null pointer constant. >>>>>> Folding is actively harmful here. >>>>>> >>>>>> And again, in C++98 mode null_ptr_cst_p already folds, so doing it here >>>>>> is redundant. >>>>>> >>>>>> Was I unclear? >> >> >> ^ See comment above. Cleaned up. >>>>>>>>> @@ -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? > > For build_enumerator the call is indeed superflous. I modified build_enumerator not to fold additionally. >>>>>>>>> @@ -441,7 +441,7 @@ build_aggr_init_expr (tree type, tree init) >>>>>>>>> else if (TREE_CODE (init) == AGGR_INIT_EXPR) >>>>>>>>> fn = AGGR_INIT_EXPR_FN (init); >>>>>>>>> else >>>>>>>>> - return convert (type, init); >>>>>>>>> + return fold (convert (type, init)); >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Why fold here? >>>>>>> >>>>>>> >>>>>>> >>>>>>> We had this already in prior thread. fold (convert ()) != >>>>>>> fold_convert () for C++. The fold is just there to make sure we fold >>>>>>> away useless casts. >>>>>> >>>>>> >>>>>> But why here? Can't we fold away useless casts earlier (in convert) or >>>>>> later (when we care about having a simplified expression)? >> >> >> ^ I removed that fold. >>>>>>>>> @@ -3664,6 +3660,10 @@ convert_arguments (tree typelist, vec>>>>>>>> va_gc> >>>>>>>>> **values, tree fndecl, >>>>>>>>> && (type == 0 || TREE_CODE (type) != REFERENCE_TYPE)) >>>>>>>>> val = TREE_OPERAND (val, 0); >>>>>>>>> >>>>>>>>> + /* For BUILT_IN_NORMAL we want to fold constants. */ >>>>>>>>> + if (fndecl && DECL_BUILT_IN (fndecl) >>>>>>>>> + && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) >>>>>>>>> + val = fold (val); >>>>>>>> >>>>>>>> >>>>>>>> Why? >>>>>>> >>>>>>> >>>>>>> As builtin-handlers are expecting to see constant values. >>>>> >>>>> >>>>> I would think this should be maybe_constant_value then. >>>> >>>> >>>> Why? At the end we resolve normal-builtin via 'fold_call_expr'. Of >>>> course we can invoke here maybe_constant_value, but it would end up in >>>> the same folding of a builtin-expression. So calling here directly >>>> 'fold' just short-cuts this. >>> >>> >>> ? >> >> >> Wait. Why are we folding here, again? Which builtins need to have constant >> values here, before late folding? > > Well, I would have assumed here first that on builitin-functions, we > need to fold arguments (at least the constant values), as > builtin-folder-routines are depending on seeing them (eg. > builtin_expect, etc). But by looking on code, I would assume that > this method doesn't do this ... > I am confused. So I would assume that this fold in convert_arguments > for builtin-normal functions seems not to be necessary. We should > handle this at other places already (and better). I removed this hunk, and retested. It isn't necessary anymore, so I removed it. >>>>>> @@ -867,7 +867,7 @@ expand_subword_shift (machine_mode op1_mode, optab >>>>>> binoptab, >>>>>> are truncated to the mode size. */ >>>>>> carries = expand_binop (word_mode, reverse_unsigned_shift, >>>>>> outof_input, const1_rtx, 0, unsignedp, >>>>>> methods); >>>>>> - if (shift_mask == BITS_PER_WORD - 1) >>>>>> + if (shift_mask == (unsigned HOST_WIDE_INT) (BITS_PER_WORD - 1)) >>>>> >>>>> >>>>> >>>>> These should still be unnecessary. > > No more. > >> Looks like this is still there. > > Right, didn't noticed that I haven't caught them too, while cleaning > those no longer required signed/unsigned cast modifications for > bootstrap. Removed it. >>>>>>>>> @@ -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'. >>>>>>> @@ -3311,6 +3311,9 @@ finish_case_label (location_t loc, tree >>>>>>> low_value, tree hi >>>>>>> gh_value) >>>>>>> low_value = case_conversion (type, low_value); >>>>>>> high_value = case_conversion (type, high_value); >>>>>>> >>>>>>> + low_value = cp_fully_fold (low_value); >>>>>>> + high_value = cp_fully_fold (high_value); >>>>>> >>>>>> >>>>>> >>>>>> Again, case_conversion should have already folded constants. >> > > Yes, folding is here superflous. I will remove it. Removed it from branch. Kai