From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 83379 invoked by alias); 27 Jul 2015 18:36:53 -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 83370 invoked by uid 89); 27 Jul 2015 18:36:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_50,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS 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; Mon, 27 Jul 2015 18:36:50 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 092A7B1FAB; Mon, 27 Jul 2015 18:36:48 +0000 (UTC) Received: from [10.10.116.43] ([10.10.116.43]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t6RGpnlt007391; Mon, 27 Jul 2015 12:51:49 -0400 Subject: Re: C++ delayed folding branch review To: Kai Tietz References: <557A5214.7060106@redhat.com> <1424811417.1214725.1434125493982.JavaMail.zimbra@redhat.com> <557BAE5A.7030309@redhat.com> Cc: Kai Tietz , gcc-patches List From: Jason Merrill Message-ID: <55B661A1.6090308@redhat.com> Date: Mon, 27 Jul 2015 19:01:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <557BAE5A.7030309@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2015-07/txt/msg02289.txt.bz2 I've trimmed this to the previously mentioned issues that still need to be addressed; I'll do another full review after these are dealt with. On 06/13/2015 12:15 AM, Jason Merrill wrote: > On 06/12/2015 12:11 PM, Kai Tietz wrote: >>>> @@ -1052,6 +1054,9 @@ adjust_temp_type (tree type, tree temp) >>>> { >>>> if (TREE_TYPE (temp) == type) >>>> return temp; >>>> + STRIP_NOPS (temp); >>>> + if (TREE_TYPE (temp) == type) >>>> + return temp; >>>> @@ -1430,6 +1438,8 @@ cxx_eval_call_expression (const constexpr_ctx >>>> *ctx, >>>> tree t, >>>> bool >>>> reduced_constant_expression_p (tree t) >>>> { >>>> + /* Make sure we remove useless initial NOP_EXPRs. */ >>>> + STRIP_NOPS (t); >>> >>> Within the constexpr code we should be folding away NOPs as they are >>> generated, they shouldn't live this long. >> >> Well, we might see them on overflows ... > > We shouldn't within the constexpr code. NOPs for expressions that are > non-constant due to overflow are added in > cxx_eval_outermost_constant_expr, so we shouldn't see them in the middle > of constexpr evaluation. > >>>> @@ -1088,7 +1093,10 @@ cxx_bind_parameters_in_call (const constexpr_ctx >>>> *ctx, tree t, >>>> && is_dummy_object (x)) >>>> { >>>> x = ctx->object; >>>> - x = cp_build_addr_expr (x, tf_warning_or_error); >>>> + if (x) >>>> + x = cp_build_addr_expr (x, tf_warning_or_error); >>>> + else >>>> + x = get_nth_callarg (t, i); >>> >>> This still should not be necessary. >> >> Yeah, most likely. But I got initially here some issues, so I don't >> see that this code would worsen things. > > If this code path is hit, that means something has broken my design, and > I don't want to just paper over that. Please revert this change. > >>>> case SIZEOF_EXPR: >>>> + if (processing_template_decl >>>> + && (!COMPLETE_TYPE_P (TREE_TYPE (t)) >>>> + || TREE_CODE (TYPE_SIZE (TREE_TYPE (t))) != INTEGER_CST)) >>>> + return t; >>> >>> Why is this necessary? >> >> We don't want to resolve SIZEOF_EXPR within template-declarations for >> incomplete types, of if its size isn't fixed. Issue is that we >> otherwise get issues about expressions without existing type (as usual >> within template-declarations for some expressions). > > Yes, but we shouldn't have gotten this far with a dependent sizeof; > maybe_constant_value just returns if > instantiation_dependent_expression_p is true. > >>>> @@ -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. > >>>> @@ -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. > >>>> @@ -608,9 +608,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); >>> >>> I still think that cp_fold_convert should always call fold_convert, and >>> callers that we don't want to fold should call convert instead, or >>> another function that folds only conversion of constants. We had talked >>> about the name "fold_cst", but I think that name isn't very clear; would >>> it make sense to just have convert always fold conversions of constants? >> >> We could introduce that, but we still have the issues about some >> unary-operations on constants, too. So we could do for any conversion >> afterwards a call to cp_try_fold_to_constant, which should reflect >> that pretty well, beside within template-declarations ... Now we've been talking about calling it "fold_simple". >>>> @@ -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. > >>>> @@ -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? > >>>> @@ -8496,16 +8467,18 @@ compute_array_index_type (tree name, tree size, >>>> tsubst_flags_t complain) >>>> SET_TYPE_STRUCTURAL_EQUALITY (itype); >>>> return itype; >>>> } >>>> - >>>> + >>>> + /* We need to do fully folding to determine if we have VLA, or >>>> not. */ >>>> + tree size_constant = cp_try_fold_to_constant (size); >>> >>> Again, we already called maybe_constant_value. >> >> Sure, but maybe_constant_value still produces nops ... > > If someone tries to create an array with a size that involves arithmetic > overflow, that's undefined behavior and we should probably give an error > rather than fold it away. > >>>> @@ -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. >>>> @@ -13102,6 +13068,7 @@ build_enumerator (tree name, tree value, tree >>>> enumtype, tree attributes, >>>> if (value != NULL_TREE) >>>> { >>>> value = cxx_constant_value (value); >>>> + STRIP_NOPS (value); >>> >>> Again, the only time a constant result should have a NOP_EXPR around it >>> is if it isn't really constant. Why do you want to strip that? >> >> As for an enumerator-value we might have overflows, which are silently >> ignored. > > They shouldn't be ignored. C++ requires that the value be constant, and > overflow makes it non-constant. > >> I will recheck this what example we have for this when bootstrap is >> working again. >> >>>> @@ -6575,6 +6578,13 @@ cp_parser_postfix_open_square_expression >>>> (cp_parser >>>> *parser, >>>> index = cp_parser_expression (parser); >>>> } >>>> >>>> + /* For offsetof and declaration of types we need >>>> + constant integeral values. >>>> + Also we meed to fold for negative constants so that diagnostic in >>>> + c-family/c-common.c doesn't fail for array-bounds. */ >>>> + if (for_offsetof || decltype_p >>>> + || (TREE_CODE (index) == NEGATE_EXPR && TREE_CODE (TREE_OPERAND >>>> (index, 0)) == INTEGER_CST)) >>>> + index = cp_try_fold_to_constant (index); >>> >>> Similarly, for offsetof the folding should happen closer to where it is >>> needed. >>> >>> Why is it needed for decltype, which is querying the type of an >>> expression? >>> >>> For NEGATE_EXPR, we had talked about always folding a NEGATE of a >>> constant; this isn't the right place to do it. >> >> Same as above, we need in those cases (and for -1 too) the constant >> values early anyway. So I saw it as more logical to have done this >> conversion as soon as possible after initialization. > > I don't think this is as soon as possible; we can fold the NEGATE_EXPR > immediately when we build it, at the end of cp_build_unary_op. > > I still wonder why any folding is necessary for decltype. When I ask > why, I want to know *why*, not just have you tell me again that it's > needed. I don't think it is. > > For offsetof, I wonder if it makes sense to extend fold_offsetof_1 to > handle whatever additional folding is needed here. If not, then fold in > finish_offsetof, before calling fold_offsetof. I see that this is now an unconditional fold_simple, but I still don't understand why it needs to be folded here, in the parser. >.... >> Anyway, if you prefer, we can do this in builder-routines, and remove >> at places constants aren't needed directly after parsing it those calls. > > I want to delay it to: > > 1) the places where we actually care about constant values, all of which > already call maybe_constant_value or cxx_constant_value, so they > shouldn't need much change; and > 2) the places where we want a simplified expression for warnings, where > we should call fold_simple. > Folding in the parser is wrong, most of all because template > substitution doesn't go through the parser. There are still several folds in cp_parser_omp_* that should move later. > finish_unary_op_expr (location_t loc, enum tree_code code, tree expr, > tsubst_flags_t complain) > { > + tree expr_ovl = expr; > tree result = build_x_unary_op (loc, code, expr, complain); > + tree result_ovl = result; > + > + expr_ovl = fold_simple (expr_ovl); > + STRIP_NOPS (expr_ovl); Why both fold_simple and STRIP_NOPS? >>>> @@ -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)? > >>>> @@ -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. >>>> @@ -5026,18 +5023,21 @@ cp_build_binary_op (location_t location, >>>> } >>>> >>>> result = build2 (resultcode, build_type, op0, op1); >>>> - result = fold_if_not_in_template (result); >>>> if (final_type != 0) >>>> result = cp_convert (final_type, result, complain); >>>> - >>>> - if (TREE_OVERFLOW_P (result) >>>> + op0 = fold_non_dependent_expr (op0); >>>> + op1 = fold_non_dependent_expr (op1); >>>> + STRIP_NOPS (op0); >>>> + STRIP_NOPS (op1); >>>> + result_ovl = fold_build2 (resultcode, build_type, op0, op1); >>>> + if (TREE_OVERFLOW_P (result_ovl) >>>> && !TREE_OVERFLOW_P (op0) >>>> && !TREE_OVERFLOW_P (op1)) >>>> - overflow_warning (location, result); >>>> + overflow_warning (location, result_ovl); >>> >>> Don't you want to use cp_fully_fold here? > > ? Introducing *_non_dependent_expr is definitely wrong here. >>>> @@ -7249,7 +7249,7 @@ gimplify_omp_for (tree *expr_p, gimple_seq >>>> *pre_p) >>>> /* Handle OMP_FOR_COND. */ >>>> t = TREE_VEC_ELT (OMP_FOR_COND (for_stmt), i); >>>> gcc_assert (COMPARISON_CLASS_P (t)); >>>> - gcc_assert (TREE_OPERAND (t, 0) == decl); >>>> + gcc_assert (TREE_OPERAND (t, 0) == decl || TREE_OPERAND (t, >>>> 1) == >>>> decl); >>> >>> Why didn't delayed folding canonicalize this so that the decl is in op0? >> >> Delay folding doesn't canonicalize this. > > Why not? Doesn't it fold all expressions? ? >> Actually we don't want to touch here anything in parsered tree. We >> could do this in generalization-pass before gimplification. Seems to >> be something we don't catch for now, which makes me wonder a bit. >> >>>> @@ -508,7 +508,9 @@ extract_omp_for_data (gomp_for *for_stmt, struct >>>> omp_for_data *fd, >>>> gcc_assert (gimple_omp_for_kind (for_stmt) >>>> == GF_OMP_FOR_KIND_CILKSIMD >>>> || (gimple_omp_for_kind (for_stmt) >>>> - == GF_OMP_FOR_KIND_CILKFOR)); >>>> + == GF_OMP_FOR_KIND_CILKFOR) >>>> + || (gimple_omp_for_kind (for_stmt) >>>> + == GF_OMP_FOR_KIND_FOR)); >>> >>> This still seems like a red flag; how is delayed folding changing the >>> OMP for kind? >> >> It doesn't. The issue is that some canonical operations of fold >> aren't happening anymore on which omp depends. > > That seems like a problem. > @@ -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. >>>> @@ -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); > 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. >>>> @@ -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. > > It looks like your latest checkin added more redundant folding: > >> @@ -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. > >> @@ -5776,6 +5776,8 @@ convert_nontype_argument (tree type, tree expr, >> tsubst_flags_t complain) >> { >> tree expr_type; >> >> + expr = cp_try_fold_to_constant (expr); >> + >> /* Detect immediately string literals as invalid non-type argument. >> This special-case is not needed for correctness (we would easily >> catch this later), but only to provide better diagnostic for this >> @@ -5852,6 +5854,7 @@ convert_nontype_argument (tree type, tree expr, >> tsubst_flags_t complain) >> else if (TYPE_PTR_OR_PTRMEM_P (type)) >> { >> tree folded = maybe_constant_value (expr); >> + folded = cp_try_fold_to_constant (expr); > > And here, convert_nontype_argument already uses > maybe_constant_value/cxx_constant_value for folding constants. Jason