From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 90003 invoked by alias); 12 Jun 2015 16:11:42 -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 89984 invoked by uid 89); 12 Jun 2015 16:11:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx3-phx2.redhat.com Received: from mx3-phx2.redhat.com (HELO mx3-phx2.redhat.com) (209.132.183.24) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 12 Jun 2015 16:11:36 +0000 Received: from zmail14.collab.prod.int.phx2.redhat.com (zmail14.collab.prod.int.phx2.redhat.com [10.5.83.16]) by mx3-phx2.redhat.com (8.13.8/8.13.8) with ESMTP id t5CGBYxa025105; Fri, 12 Jun 2015 12:11:34 -0400 Date: Fri, 12 Jun 2015 16:17:00 -0000 From: Kai Tietz To: Jason Merrill Cc: Kai Tietz , gcc-patches List Message-ID: <1424811417.1214725.1434125493982.JavaMail.zimbra@redhat.com> In-Reply-To: <557A5214.7060106@redhat.com> References: <557A5214.7060106@redhat.com> Subject: Re: C++ delayed folding branch review MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2015-06/txt/msg00941.txt.bz2 Hello Jason, Thanks for the review. I addressed a lot of your comments directly on svn-= branch. See revision r224439. ----- Urspr=C3=BCngliche Mail ----- > Generally, it seems like most of my comments from April haven't been > addressed yet. Yes, most of them. =20 > > @@ -3023,13 +3023,14 @@ conversion_warning (location_t loc, tree type, = tree > > expr >=20 > Instead of adding folds here, let's make sure that the argument we pass > (e.g. from cp_convert_and_check to warnings_for_convert_and_check) is > fully folded. I looked for dependencies, and address this. =20 > > @@ -589,9 +589,9 @@ null_member_pointer_value_p (tree t) > > return false; > > else if (TYPE_PTRMEMFUNC_P (type)) > > return (TREE_CODE (t) =3D=3D 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)); >=20 > Again, calling fold here is wrong; it doesn't handle constexpr, and we > should have folded before we got here. Agreed. I will commit change for this. Nevertheless CONSTRUCTOR_ELT's value might still be prefixed by nops due po= ssible overflows, or by negative sign/invert/etc. This is caused by non-co= nstant-folding of values. I removed for now those folds, as I agree we sho= uld address this at place of value-assignment. Nevertheless we still have = this issue =20 > > @@ -5090,9 +5090,9 @@ build_conditional_expr_1 (location_t loc, tree ar= g1, > > tree arg2, tree arg3, > > > > valid_operands: > > result =3D build3 (COND_EXPR, result_type, arg1, arg2, arg3); > > - if (!cp_unevaluated_operand) > > + if (!cp_unevaluated_operand && !processing_template_decl) > > /* Avoid folding within decltype (c++/42013) and noexcept. */ > > - result =3D fold_if_not_in_template (result); > > + result =3D fold (result); >=20 > This seems related to your status report note: >=20 > > Additionally addressed issue about cond_expr, as we want to fold cases = with > > a constant-condition. Here we need to use "fold_to_constant" so that we > > just fold things to constant-value, if possible and otherwise don't mod= ify > > anything. >=20 > But why do you say we want to fold cases with a constant condition? We > certainly want to avoid warning about the dead branch in that case, but > it would be better if we can do that folding only in the warning code. Issue is that we otherwise detect in conditions that expressions aren't con= stant due never-executed-code-path. The diagnostics we can deal differentl= y, but this was actually the reason for doing this. I can remove this here= , but we still need a place to avoid ill detection of constexpr (or invalid= code) on dead code-branch. Eg. (1 ? 0/0 : 1) etc =20 > > @@ -5628,8 +5628,8 @@ build_new_op_1 (location_t loc, enum tree_code co= de, > > int f > > lags, tree arg1, > > decaying an enumerator to its value. */ > > if (complain & tf_warning) > > warn_logical_operator (loc, code, boolean_type_node, > > - code_orig_arg1, arg1, > > - code_orig_arg2, arg2); > > + code_orig_arg1, fold (arg1), > > + code_orig_arg2, fold (arg2)); > > > > arg2 =3D convert_like (conv, arg2, complain); > > } > > @@ -5666,7 +5666,8 @@ build_new_op_1 (location_t loc, enum tree_code co= de, > > int f > > lags, tree arg1, > > case TRUTH_AND_EXPR: > > case TRUTH_OR_EXPR: > > warn_logical_operator (loc, code, boolean_type_node, > > - code_orig_arg1, arg1, code_orig_arg2, arg2= ); > > + code_orig_arg1, fold (arg1), > > + code_orig_arg2, fold (arg2)); > > /* Fall through. */ > > case GT_EXPR: > > case LT_EXPR: > > @@ -5676,7 +5677,7 @@ build_new_op_1 (location_t loc, enum tree_code co= de, > > int f > > lags, tree arg1, > > case NE_EXPR: > > if ((code_orig_arg1 =3D=3D BOOLEAN_TYPE) > > ^ (code_orig_arg2 =3D=3D BOOLEAN_TYPE)) > > - maybe_warn_bool_compare (loc, code, arg1, arg2); > > + maybe_warn_bool_compare (loc, code, fold (arg1), fold (arg2)); > > /* Fall through. */ > > case PLUS_EXPR: > > case MINUS_EXPR: >=20 > I still think these fold calls should be cp_fully_fold. Ok, modified here use of fold to cp_fully_fold. >=20 > > @@ -7382,8 +7383,13 @@ build_over_call (struct z_candidate *cand, int > > flags, tsu > > bst_flags_t complain) > > > > gcc_assert (j <=3D nargs); > > nargs =3D j; > > + { > > + tree *fargs =3D (!nargs ? argarray : (tree *) alloca (nargs * size= of > > (tree))) > > ; > > + for (j =3D 0; j < nargs; j++) > > + fargs[j] =3D fold_non_dependent_expr (argarray[j]); >=20 > No change needed here, but I notice that fold_non_dependent_expr is > still using maybe_constant_value; it should probably use cp_fully_fold > instead. Hmm, maybe we should limit this folding on constants. So cp_fold_to_consta= nt ()? =20 > > @@ -1052,6 +1054,9 @@ adjust_temp_type (tree type, tree temp) > > { > > if (TREE_TYPE (temp) =3D=3D type) > > return temp; > > + STRIP_NOPS (temp); > > + if (TREE_TYPE (temp) =3D=3D type) > > + return temp; > > @@ -1430,6 +1438,8 @@ cxx_eval_call_expression (const constexpr_ctx *ct= x, > > tree t, > > bool > > reduced_constant_expression_p (tree t) > > { > > + /* Make sure we remove useless initial NOP_EXPRs. */ > > + STRIP_NOPS (t); >=20 > 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 ... =20 > > @@ -1088,7 +1093,10 @@ cxx_bind_parameters_in_call (const constexpr_ctx > > *ctx, tree t, > > && is_dummy_object (x)) > > { > > x =3D ctx->object; > > - x =3D cp_build_addr_expr (x, tf_warning_or_error); > > + if (x) > > + x =3D cp_build_addr_expr (x, tf_warning_or_error); > > + else > > + x =3D get_nth_callarg (t, i); >=20 > This still should not be necessary. Yeah, most likely. But I got initially here some issues, so I don't see th= at this code would worsen things. =20 > > @@ -1576,13 +1586,15 @@ cxx_eval_unary_expression (const constexpr_ctx > > *ctx, tre > > e t, > > enum tree_code code =3D TREE_CODE (t); > > tree type =3D TREE_TYPE (t); > > r =3D fold_unary_loc (loc, code, type, arg); > > - if (r =3D=3D NULL_TREE) > > + if (r =3D=3D NULL_TREE || !CONSTANT_CLASS_P (r)) > > { > > if (arg =3D=3D orig_arg) > > r =3D t; > > else > > r =3D build1_loc (loc, code, type, arg); > > } > > + else > > + r =3D unify_constant (ctx, r, overflow_p); >=20 > This still should not be necessary. Well, I just wanted to make sure that if arg is a PTRMEM_CST (or something = like this) that we still try to resolve its constant. But yes, you are rig= ht that arg is anyway resolved already, and so this might not happen. Neve= rtheless we might need to handle here OVERFLOW? =20 > > @@ -1780,7 +1792,8 @@ cxx_eval_component_reference (const constexpr_ctx > > *ctx, tree t, > > if (field =3D=3D part) > > { > > if (value) > > - return value; > > + return cxx_eval_constant_expression (ctx, value, lval, > > + non_constant_p, > > overflow_p); > > else > > /* We're in the middle of initializing it. */ > > break; > > @@ -1864,7 +1877,8 @@ cxx_eval_bit_field_ref (const constexpr_ctx *ctx, > > tree t, > > { > > tree bitpos =3D bit_position (field); > > if (bitpos =3D=3D start && DECL_SIZE (field) =3D=3D TREE_OPERAND= (t, 1)) > > - return value; > > + return cxx_eval_constant_expression (ctx, value, lval, > > + non_constant_p, overflow_= p); > > if (TREE_CODE (TREE_TYPE (field)) =3D=3D INTEGER_TYPE > > && TREE_CODE (value) =3D=3D INTEGER_CST > > && tree_fits_shwi_p (bitpos) >=20 > Again, this shouldn't be necessary, either; the elements of the > CONSTRUCTOR should be fully evaluated already. Evaluated yes, but not necessarily folded to constant. Maybe cp_fold_to_co= nstant could be better choice here? =20 > > @@ -2987,19 +3017,15 @@ cxx_eval_constant_expression (const constexpr_c= tx > > *ctx, tree t, > > constexpr_ctx new_ctx; > > tree r =3D t; > > > > - if (t =3D=3D error_mark_node) > > + if (!t || t =3D=3D error_mark_node) > > { > > *non_constant_p =3D true; > > - return t; > > + return error_mark_node; > > } > > @@ -3761,6 +3819,8 @@ fold_non_dependent_expr (tree t) > > tree > > maybe_constant_init (tree t, tree decl) > > { > > + if (!t) > > + return t; >=20 > You were going to test whether the null check was necessary. What was > the result? I will need to redo this test if the boostrap-issue about real.h is resolve= d. But I agree that this check could be superflous. =20 > > case SIZEOF_EXPR: > > + if (processing_template_decl > > + && (!COMPLETE_TYPE_P (TREE_TYPE (t)) > > + || TREE_CODE (TYPE_SIZE (TREE_TYPE (t))) !=3D INTEGER_CST)) > > + return t; >=20 > Why is this necessary? We don't want to resolve SIZEOF_EXPR within template-declarations for incom= plete types, of if its size isn't fixed. Issue is that we otherwise get is= sues about expressions without existing type (as usual within template-decl= arations for some expressions). =20 > > @@ -3368,6 +3399,15 @@ cxx_eval_constant_expression (const constexpr_ctx > > *ctx, tree t, > > /* Don't re-process a constant CONSTRUCTOR, but do fold it to > > VECTOR_CST if applicable. */ > > return fold (t); > > + /* See this can happen for case like g++.dg/init/static2.C testc= ase. > > */ > > + if (!ctx || !ctx->ctor || (lval && !ctx->object) > > + || !same_type_ignoring_top_level_qualifiers_p > > + (TREE_TYPE (t), TREE_TYPE (ctx->ctor)) > > + || CONSTRUCTOR_NELTS (ctx->ctor) !=3D 0) > > + { > > + *non_constant_p =3D true; > > + break; > > + } >=20 > Why does this happen for static2.C when it doesn't happen on the trunk? > I think the bug is somewhere else. I will try to investigate in more detail, when bootstrap is working again. =20 > > @@ -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 =3D TREE_CODE (t); > > tree oldop =3D TREE_OPERAND (t, 0); > > + > > + if (tcode =3D=3D NOP_EXPR && TREE_TYPE (t) =3D=3D TREE_TYPE (ol= dop) && > > 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 =3D true; > > + *non_constant_p =3D true; > > + > > + return t; > > + } > > tree op =3D cxx_eval_constant_expression (ctx, oldop, >=20 > 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 over= flow. As this isn't handled by cxx_eval_constant_expression. =20 > > @@ -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 =3D 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 =3D cxx_sizeof_or_alignof_type (TREE_OPERAND (*expr_p, = 0), > > + SIZEOF_EXPR, false); > > + else > > + *expr_p =3D cxx_sizeof_or_alignof_expr (TREE_OPERAND (*expr_p, = 0), > > + SIZEOF_EXPR, false); > > + if (*expr_p =3D=3D error_mark_node) > > + *expr_p =3D size_one_node; > > + > > + *expr_p =3D maybe_constant_value (*expr_p); > > + ret =3D GS_OK; > > + break; >=20 > 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. =20 > > +static tree > > +cp_fold (tree x, hash_map *fold_hash) >=20 > Looks like cp_fold still doesn't call maybe_constant_value. Yes, it should do this just in case of seen PTRMEM_CST. Or shall we try to= invoke it on each iteration within cp_fold. Later might be a bit costy, i= sn't it? =20 > > + case CALL_EXPR: > > + r =3D fold (x); > > + if (TREE_CODE (r) !=3D CALL_EXPR) > > + { > > + x =3D cp_fold (r, fold_hash); > > + break; > > + } > > + { > > + int i, m =3D call_expr_nargs (x); > > + for (i =3D 0; i < m; i++) > > + { > > + CALL_EXPR_ARG (x, i) =3D cp_fold (CALL_EXPR_ARG (x, i), fold_= hash); > > + } > > + } > > + r =3D fold (x); > > + if (TREE_CODE (r) !=3D CALL_EXPR) > > + { > > + x =3D cp_fold (r, fold_hash); > > + break; > > + } > > + return org_x; >=20 > Why return org_x here? Don't we still want to add this to the hash table? No, as we just tried here to fold expression-arguments, so that later fold = might be able to optimize. So x is intermediate, and we don't want to cach= e it. But well, we might want to hash here org_x itself ... =20 > I'm also nervous about clobbering the args in the original CALL_EXPR. > In most of cp_fold you build a new expression rather than change the > operands of the original one. Well, this shouldn't be problematic IMP. I just wanted to avoid to make us= e of a node-copy, which might be for a call-expression expensive. But well= , as we now have cp_try_fold_to_constant, this could lead indeed to folded = call-expressions too early. Before cp_fold was just used in the final fold= ing before gimplification, where these modifications weren't problematic at= all. So we might want to avoid such argument-conversion in context of cp_try_fol= d_to_constant at all? =20 > > @@ -608,9 +608,13 @@ cp_fold_convert (tree type, tree expr) > > } > > else > > { > > - conv =3D fold_convert (type, expr); > > + if (TREE_CODE (expr) =3D=3D INTEGER_CST) > > + conv =3D fold_convert (type, expr); > > + else > > + conv =3D convert (type, expr); >=20 > 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-oper= ations on constants, too. So we could do for any conversion afterwards a c= all to cp_try_fold_to_constant, which should reflect that pretty well, besi= de within template-declarations ... =20 > > @@ -632,12 +636,12 @@ cp_convert (tree type, tree expr, tsubst_flags_t > > complain) > > tree > > cp_convert_and_check (tree type, tree expr, tsubst_flags_t complain) > > { > > - tree result; > > + tree result, ret; > > > > if (TREE_TYPE (expr) =3D=3D type) > > return expr; > > > > - result =3D cp_convert (type, expr, complain); > > + result =3D ret =3D cp_convert (type, expr, complain); > > > > if ((complain & tf_warning) > > && c_inhibit_evaluation_warnings =3D=3D 0) > > @@ -646,6 +650,7 @@ cp_convert_and_check (tree type, tree expr, > > tsubst_flags_t complain) > > tree stripped =3D folded; > > tree folded_result > > =3D folded !=3D expr ? cp_convert (type, folded, complain) : re= sult; > > + folded_result =3D fold (folded_result); > > > > /* maybe_constant_value wraps an INTEGER_CST with TREE_OVERFLOW = in a > > NOP_EXPR so that it isn't TREE_CONSTANT anymore. */ > > @@ -657,7 +662,7 @@ cp_convert_and_check (tree type, tree expr, > > tsubst_flags_t complain) > > folded_result); > > } > > > > - return result; > > + return ret; > > } >=20 > Again, there seems to be no reason to introduce the new "ret" variable. Yes, removed it. I am trying right now a version using here cp_fully_fold instead of fold. =20 > > @@ -1529,8 +1532,11 @@ build_expr_type_conversion (int desires, tree ex= pr, > > bool complain) > > tree basetype =3D TREE_TYPE (expr); > > tree conv =3D NULL_TREE; > > tree winner =3D NULL_TREE; > > + /* Want to see if EXPR is a constant. See below checks for null_nod= e. > > */ > > + tree expr_folded =3D cp_try_fold_to_constant (expr); > > > > - if (expr =3D=3D null_node > > + STRIP_NOPS (expr_folded); > > + if (expr_folded =3D=3D null_node >=20 > 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. 6Well, we need to do this for diagnostic messages AFAIR. We want to see if= expression folded gets a constant, so that diagnostics getting displayed r= ight. =20 > > @@ -1548,7 +1554,7 @@ build_expr_type_conversion (int desires, tree exp= r, > > 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)) >=20 > 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 ... =20 > > @@ -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 =3D cp_try_fold_to_constant (size); >=20 > Again, we already called maybe_constant_value. Sure, but maybe_constant_value still produces nops ... =20 > > @@ -8736,7 +8697,10 @@ create_array_type_for_decl (tree name, tree type, > > tree size) > > > > /* Figure out the index type for the array. */ > > if (size) > > - itype =3D compute_array_index_type (name, size, tf_warning_or_erro= r); > > + { > > + size =3D cp_try_fold_to_constant (size); > > + itype =3D compute_array_index_type (name, size, tf_warning_or_er= ror); > > + } >=20 > There's no need to fold here and inside compute_array_index_type. Yeahm within compute_array_index_type should be enough. =20 > > @@ -13078,6 +13042,8 @@ build_enumerator (tree name, tree value, tree > > enumtype, tree attributes, > > if (value) > > STRIP_TYPE_NOPS (value); > > > > + if (value) > > + value =3D cp_try_fold_to_constant (value); >=20 > Again, this is unnecessary because we call cxx_constant_value below. See nops, and other unary-operations we want to reduce here to real constan= t value ... =20 > > @@ -13102,6 +13068,7 @@ build_enumerator (tree name, tree value, tree > > enumtype, tree attributes, > > if (value !=3D NULL_TREE) > > { > > value =3D cxx_constant_value (value); > > + STRIP_NOPS (value); >=20 > 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 igno= red. I will recheck this what example we have for this when bootstrap is w= orking again. =20 > > @@ -779,7 +779,8 @@ perform_member_init (tree member, tree init) > > in that case. */ > > init =3D build_x_compound_expr_from_list (init, ELK_MEM_INIT, > > tf_warning_or_error); > > - > > + if (init) > > + init =3D fold (init); >=20 > Again, why fold here, rather than later when something really wants a > constant? If that ever actually occurs? Hmm, this had to do with some weak code-generation AFAIR. Need to recheck.= Could be also a relict about unary-operations on constants, which we want= to elliminate for initializers soon. =20 > > @@ -417,7 +417,9 @@ strip_using_decl (tree decl) > > if (decl =3D=3D NULL_TREE) > > return NULL_TREE; > > > > - while (TREE_CODE (decl) =3D=3D USING_DECL && !DECL_DEPENDENT_P (decl= )) > > + while (TREE_CODE (decl) =3D=3D USING_DECL > > + && !DECL_DEPENDENT_P (decl) > > + && !uses_template_parms (USING_DECL_SCOPE (decl))) >=20 > This seems unrelated to delayed folding. Yeah, I will remove from branch. Should be nevertheless something pretty = opaque ... =20 > > @@ -6136,6 +6139,10 @@ push_to_top_level (void) > > else > > need_pop =3D false; > > > > + if (scope_chain) > > + fm =3D scope_chain->fold_map; > > + else > > + fm =3D NULL; > > if (scope_chain && previous_class_level) > > store_class_bindings (previous_class_level->class_shadowed, > > &s->old_bindings); > > @@ -6167,6 +6174,9 @@ push_to_top_level (void) > > FOR_EACH_VEC_SAFE_ELT (s->old_bindings, i, sb) > > IDENTIFIER_MARKED (sb->identifier) =3D 0; > > > > + if (!fm) > > + fm =3D new hash_map; >=20 > Why are these hunks so far apart? I would expect them to be all together. Oh, nothing special. I just wanted to have base initialization together wi= th the other if initializes, and doing then later near assignment the optio= nal allocation. I can change this, if you prefer. =20 > > @@ -6473,7 +6473,8 @@ cp_parser_array_notation (location_t loc, cp_pars= er > > *parser, tree *init_index, > > 2. ARRAY [ EXP : EXP ] > > 3. ARRAY [ EXP : EXP : EXP ] */ > > > > - *init_index =3D cp_parser_expression (parser); > > + *init_index =3D cp_parser_expression (parser); > > + *init_index =3D cp_try_fold_to_constant (*init_index); > > if (cp_lexer_peek_token (parser->lexer)->type !=3D CPP_COLON) > > { > > /* This indicates that we have a normal array expression. */ > > @@ -6484,10 +6485,12 @@ cp_parser_array_notation (location_t loc, cp_pa= rser > > *parser, tree *init_index, > > /* Consume the ':'. */ > > cp_lexer_consume_token (parser->lexer); > > length_index =3D cp_parser_expression (parser); > > + length_index =3D cp_try_fold_to_constant (length_index); > > if (cp_lexer_peek_token (parser->lexer)->type =3D=3D CPP_COLON) > > { > > cp_lexer_consume_token (parser->lexer); > > stride =3D cp_parser_expression (parser); > > + stride =3D cp_try_fold_to_constant (stride); >=20 > Again, why fold here, rather than later when something really wants a > constant? If that ever actually occurs? This code for cilk expects that these statements are folded to constants. = It doesn't make much difference to move those folders to later locations, a= s we have to fold them early anyway. > > @@ -6575,6 +6578,13 @@ cp_parser_postfix_open_square_expression (cp_par= ser > > *parser, > > index =3D 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) =3D=3D NEGATE_EXPR && TREE_CODE (TREE_OPER= AND > > (index, 0)) =3D=3D INTEGER_CST)) > > + index =3D cp_try_fold_to_constant (index); >=20 > Similarly, for offsetof the folding should happen closer to where it is > needed. >=20 > Why is it needed for decltype, which is querying the type of an expressio= n? >=20 > 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. > > @@ -8031,7 +8041,9 @@ cp_parser_cast_expression (cp_parser *parser, bool > > address_p, bool cast_p, > > return error_mark_node; > > > > /* Perform the cast. */ > > - expr =3D build_c_cast (input_location, type, expr); > > + /* We don't want to resolve cast too early. Therefore we > > don't > > + be able to use build_c_cast. */ > > + expr =3D cp_build_c_cast (type, expr, tf_warning_or_error= ); >=20 > Huh? build_c_cast just calls cp_build_c_cast. Yes, as we don't want to do cast-folding too early. =20 > > @@ -9869,6 +9881,7 @@ cp_parser_label_for_labeled_statement (cp_parser* > > parser, tree attributes) > > cp_lexer_consume_token (parser->lexer); > > /* Parse the constant-expression. */ > > expr =3D cp_parser_constant_expression (parser); > > + expr =3D cp_try_fold_to_constant (expr); > > if (check_for_bare_parameter_packs (expr)) > > expr =3D error_mark_node; > > > > @@ -9878,6 +9891,7 @@ cp_parser_label_for_labeled_statement (cp_parser* > > parser, tree attributes) > > /* Consume the `...' token. */ > > cp_lexer_consume_token (parser->lexer); > > expr_hi =3D cp_parser_constant_expression (parser); > > + expr_hi =3D cp_try_fold_to_constant (expr_hi); >=20 > Again, this seems redundant with the call to cxx_constant_value in > case_conversion. Well, but we want to do here check_for_bare_parameter_packs directly after = that, and so we need folded value early. =20 > > @@ -12210,6 +12224,10 @@ cp_parser_static_assert(cp_parser *parser, bool > > member_p) > > /*allow_non_constant_p=3D*/true, > > /*non_constant_p=3D*/&dummy); > > > > + /* Make sure we folded it completely before doing trying to get > > + constant value. */ > > + condition =3D fold_non_dependent_expr (condition); >=20 > Again, this is redundant with the code in finish_static_assert. Hmm, yes. We check that. =20 > > @@ -16115,6 +16133,7 @@ cp_parser_enumerator_definition (cp_parser* par= ser, > > tree type) > > cp_lexer_consume_token (parser->lexer); > > /* Parse the value. */ > > value =3D cp_parser_constant_expression (parser); > > + value =3D cp_try_fold_to_constant (value); >=20 > Again, this is redundant with the code in build_enumerator. Hmm, don't see here redundancy. I just see that we need folded-value for l= eter call of check_for_bare_parameter_packs. But might not see the obvious =20 > > @@ -17702,7 +17721,8 @@ cp_parser_direct_declarator (cp_parser* parser, > > constant-expression. */ > > if (token->type !=3D CPP_CLOSE_SQUARE) > > { > > - bool non_constant_p; > > + bool non_constant_p =3D false; >=20 > This is unnecessary, as cp_parser_constant_expression will set > non_constant_p. Ok. =20 > > @@ -19354,12 +19374,10 @@ cp_parser_initializer_clause (cp_parser* pars= er, > > bool* non_constant_p) > > /* If it is not a `{', then we are looking at an > > assignment-expression. */ > > if (cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE)) > > - { > > - initializer > > - =3D cp_parser_constant_expression (parser, > > - /*allow_non_constant_p=3D*/true, > > - non_constant_p); > > - } > > + initializer > > + =3D cp_parser_constant_expression (parser, > > + /*allow_non_constant_p=3D*/true, > > + non_constant_p); >=20 > Let's not reformat unrelated code in a big project like this. Well, but it violates coding-rules. anyway agreed ;) =20 > > @@ -20955,9 +20973,11 @@ cp_parser_member_declaration (cp_parser* parse= r) > > > > /* Consume the `:' token. */ > > cp_lexer_consume_token (parser->lexer); > > + > > /* Get the width of the bitfield. */ > > width > > =3D cp_parser_constant_expression (parser); > > + width =3D maybe_constant_value (width); >=20 > Again, this seems redundant with the call to cxx_constant_value in > check_bitfield_decl. Ah, this escaped. This should be a cp_try_fold_to_constant. =20 > Again, it seems like you added maybe_constant_value or > cp_try_fold_to_constant after every occurrence of > cp_parser_constant_expression, and I suspect that few are actually > needed, and the ones that are should go closer to the code that really > needs a constant. I'd prefer to avoid calling it at all in parser.c. Actually it should be cp_try_fold_to_constant, as at those places we need t= o deal anyway with constant-values. At some please we use those values alr= eady for some diagnostics, so I thoght it is more consistent to do this fol= ding to constant directly after parsing it. To delay that into builder-rou= tines is IMO just less clear, and could lead to double-doing foldings. Add= itionally the chance to conflict here with shared parts with C is much less. Anyway, if you prefer, we can do this in builder-routines, and remove at pl= aces constants aren't needed directly after parsing it those calls. =20 > > finish_unary_op_expr (location_t loc, enum tree_code code, tree expr, > > tsubst_flags_t complain) > > { > > + tree expr_ovl =3D expr; > > tree result =3D build_x_unary_op (loc, code, expr, complain); > > + tree result_ovl =3D result; > > + > > + STRIP_NOPS (expr_ovl); > > + switch (code) > > + { > > + case ABS_EXPR: > > + case NEGATE_EXPR: > > + if (TREE_CODE (expr) =3D=3D INTEGER_CST > > + || TREE_CODE (expr) =3D=3D REAL_CST > > + || TREE_CODE (expr) =3D=3D VECTOR_CST > > + || TREE_CODE (expr) =3D=3D FIXED_CST > > + || TREE_CODE (expr) =3D=3D COMPLEX_CST) > > + result_ovl =3D fold (result); > > + break; > > + default: > > + break; > > + } >=20 > Not cp_fully_fold? Hmm, yeah, I changed the switch to a call to cp_try_fold_to_constant, whic= h should be pretty exactly what we are looking here for diagnostics. =20 > > @@ -6301,8 +6321,9 @@ handle_omp_for_class_iterator (int i, location_t > > locus, tree declv, tree initv, > > tf_warning_or_error); > > if (error_operand_p (iter_incr)) > > return true; > > - else if (TREE_CODE (incr) =3D=3D PREINCREMENT_EXPR > > - || TREE_CODE (incr) =3D=3D POSTINCREMENT_EXPR) > > + iter_incr =3D fold (iter_incr); > > + if (TREE_CODE (incr) =3D=3D PREINCREMENT_EXPR > > + || TREE_CODE (incr) =3D=3D POSTINCREMENT_EXPR) > > @@ -6357,6 +6376,7 @@ handle_omp_for_class_iterator (int i, location_t > > locus, tree declv, tree initv, > > tf_warning_or_error); > > if (error_operand_p (iter_incr)) > > return true; > > + iter_incr =3D fold (iter_incr); > > iter_incr =3D build_x_modify_expr (EXPR_LOCATION (rhs= ), > > iter, NOP_EXPR, > > iter_incr, > > @@ -6364,6 +6384,7 @@ handle_omp_for_class_iterator (int i, location_t > > locus, tree declv, tree initv, > > if (error_operand_p (iter_incr)) > > return true; > > incr =3D TREE_OPERAND (rhs, 0); > > + incr =3D fold (incr); >=20 > Why are these folds needed? >=20 > > @@ -441,7 +441,7 @@ build_aggr_init_expr (tree type, tree init) > > else if (TREE_CODE (init) =3D=3D AGGR_INIT_EXPR) > > fn =3D AGGR_INIT_EXPR_FN (init); > > else > > - return convert (type, init); > > + return fold (convert (type, init)); >=20 > Why fold here? We had this already in prior thread. fold (convert ()) !=3D fold_convert (= ) for C++. The fold is just there to make sure we fold away useless casts. =20 > > @@ -3664,6 +3660,10 @@ convert_arguments (tree typelist, vec > > **values, tree fndecl, > > && (type =3D=3D 0 || TREE_CODE (type) !=3D REFERENCE_TYPE)) > > val =3D TREE_OPERAND (val, 0); > > > > + /* For BUILT_IN_NORMAL we want to fold constants. */ > > + if (fndecl && DECL_BUILT_IN (fndecl) > > + && DECL_BUILT_IN_CLASS (fndecl) =3D=3D BUILT_IN_NORMAL) > > + val =3D fold (val); >=20 > Why? As builtin-handlers are expecting to see constant values. Otherwise the pr= oduce either ICE, or other funny things. We could call here instead also t= he cp_try_fold... function. Btw some lines above there is a comment about NOP_EXPR being extended to in= dicate that a value isn't lvalue. Not sure if this NOP_EXPR strip is still= necessary, as we don't call anymore directly into build_c_cast (). I need= to check if cp-version does the same. =20 > > @@ -4941,7 +4938,7 @@ cp_build_binary_op (location_t location, > > from being kept in a register. > > Instead, make copies of the our local variables and > > pass the copies by reference, then copy them back afterwar= d. > > */ > > - tree xop0 =3D op0, xop1 =3D op1, xresult_type =3D result_type; > > + tree xop0 =3D fold (op0), xop1 =3D fold (op1), xresult_type = =3D > > result_type; >=20 > Again, this seems wrong. In fact, the whole short_compare business > seems like the sort of early folding we want to do away with. >=20 > > @@ -5026,18 +5023,21 @@ cp_build_binary_op (location_t location, > > } > > > > result =3D build2 (resultcode, build_type, op0, op1); > > - result =3D fold_if_not_in_template (result); > > if (final_type !=3D 0) > > result =3D cp_convert (final_type, result, complain); > > - > > - if (TREE_OVERFLOW_P (result) > > + op0 =3D fold_non_dependent_expr (op0); > > + op1 =3D fold_non_dependent_expr (op1); > > + STRIP_NOPS (op0); > > + STRIP_NOPS (op1); > > + result_ovl =3D 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); >=20 > Don't you want to use cp_fully_fold here? >=20 > > @@ -7989,7 +7984,6 @@ expand_ptrmemfunc_cst (tree cst, tree *delta, tree > > *pfn) > > tree binfo =3D binfo_or_else (orig_class, fn_class); > > *delta =3D build2 (PLUS_EXPR, TREE_TYPE (*delta), > > *delta, BINFO_OFFSET (binfo)); > > - *delta =3D fold_if_not_in_template (*delta); > > > > /* We set PFN to the vtable offset at which the function can be > > found, plus one (unless ptrmemfunc_vbit_in_delta, in which > > @@ -7997,23 +7991,19 @@ expand_ptrmemfunc_cst (tree cst, tree *delta, t= ree > > *pfn) > > *pfn =3D DECL_VINDEX (fn); > > *pfn =3D build2 (MULT_EXPR, integer_type_node, *pfn, > > TYPE_SIZE_UNIT (vtable_entry_type)); > > - *pfn =3D fold_if_not_in_template (*pfn); > > > > switch (TARGET_PTRMEMFUNC_VBIT_LOCATION) > > { > > case ptrmemfunc_vbit_in_pfn: > > *pfn =3D build2 (PLUS_EXPR, integer_type_node, *pfn, > > integer_one_node); > > - *pfn =3D fold_if_not_in_template (*pfn); > > break; > > > > case ptrmemfunc_vbit_in_delta: > > *delta =3D build2 (LSHIFT_EXPR, TREE_TYPE (*delta), > > *delta, integer_one_node); > > - *delta =3D fold_if_not_in_template (*delta); > > *delta =3D build2 (PLUS_EXPR, TREE_TYPE (*delta), > > *delta, integer_one_node); > > - *delta =3D fold_if_not_in_template (*delta); > > break; > > > > default: > > @@ -8021,7 +8011,6 @@ expand_ptrmemfunc_cst (tree cst, tree *delta, tree > > *pfn) > > } > > > > *pfn =3D build_nop (TYPE_PTRMEMFUNC_FN_TYPE (type), *pfn); > > - *pfn =3D fold_if_not_in_template (*pfn); >=20 > Again, I think all the calls to fold_if_not_in_template in > expand_ptrmemfunc_cst should become regular folds. Or rather, change > the build2 to fold_build2. This is very much compiler internals, and we > should only get here when folding anyway. Ok, I will check if *delta always has a valid type. As otherwise fold_buil= d2 will crash. =20 > > @@ -1866,7 +1866,8 @@ output_loc_operands (dw_loc_descr_ref loc, int > > for_eh_or_skip) > > } > > break; > > case dw_val_class_addr: > > - gcc_assert (val1->v.val_unsigned =3D=3D DWARF2_ADDR_SIZE); > > + gcc_assert (val1->v.val_unsigned > > + =3D=3D (unsigned HOST_WIDE_INT) DWARF2_ADDR_SIZE); >=20 > Again, we need to fix this warning so this change is unnecessary. Yeah, just something intermediate. It is on my list. =20 > > @@ -7249,7 +7249,7 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p) > > /* Handle OMP_FOR_COND. */ > > t =3D TREE_VEC_ELT (OMP_FOR_COND (for_stmt), i); > > gcc_assert (COMPARISON_CLASS_P (t)); > > - gcc_assert (TREE_OPERAND (t, 0) =3D=3D decl); > > + gcc_assert (TREE_OPERAND (t, 0) =3D=3D decl || TREE_OPERAND (t, = 1) =3D=3D > > decl); >=20 > Why didn't delayed folding canonicalize this so that the decl is in op0? Delay folding doesn't canonicalize this. Actually we don't want to touch h= ere anything in parsered tree. We could do this in generalization-pass bef= ore gimplification. Seems to be something we don't catch for now, which ma= kes me wonder a bit. =20 > > @@ -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) > > =3D=3D GF_OMP_FOR_KIND_CILKSIMD > > || (gimple_omp_for_kind (for_stmt) > > - =3D=3D GF_OMP_FOR_KIND_CILKFOR)); > > + =3D=3D GF_OMP_FOR_KIND_CILKFOR) > > + || (gimple_omp_for_kind (for_stmt) > > + =3D=3D GF_OMP_FOR_KIND_FOR)); >=20 > 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 hap= pening anymore on which omp depends. =20 > > @@ -1796,6 +1796,9 @@ evaluate_stmt (gimple stmt) > > && (likelyvalue =3D=3D CONSTANT || is_gimple_call (stmt) > > || (gimple_assign_single_p (stmt) > > && gimple_assign_rhs_code (stmt) =3D=3D ADDR_EXPR)) > > + && (likelyvalue =3D=3D CONSTANT || is_gimple_call (stmt) > > + || (gimple_assign_single_p (stmt) > > + && gimple_assign_rhs_code (stmt) =3D=3D ADDR_EXPR)) >=20 > You were going to revert this merge error? Sure. =20 > > @@ -1947,6 +1947,8 @@ build_complex (tree type, tree real, tree imag) > > { > > tree t =3D make_node (COMPLEX_CST); > > > > + real =3D fold (real); > > + imag =3D fold (imag); >=20 > 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 p= lace. AFAICS is the C++ FE not calling directly build_complex. So this pl= ace was the easiest way to avoid issues with things like '-' '1' etc. =20 > > @@ -5080,6 +5081,7 @@ output_constructor_bitfield (oc_local_state *loca= l, > > unsigned int bit_offset) > > while (TREE_CODE (local->val) =3D=3D VIEW_CONVERT_EXPR > > || TREE_CODE (local->val) =3D=3D NON_LVALUE_EXPR) > > local->val =3D TREE_OPERAND (local->val, 0); > > + local->val =3D fold (local->val); >=20 > Likewise. As soon as we can be sure that values getting fully_folded, or at least fol= ded for constants, we should be able to remove this. =20 > Jason >=20 Kai