From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5217 invoked by alias); 12 Jun 2015 03:29:56 -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 5124 invoked by uid 89); 12 Jun 2015 03:29:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_50,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; Fri, 12 Jun 2015 03:29:31 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 8E4882B6497; Fri, 12 Jun 2015 03:29:29 +0000 (UTC) Received: from [10.10.116.33] ([10.10.116.33]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t5C3TSNF029294; Thu, 11 Jun 2015 23:29:28 -0400 Message-ID: <557A5214.7060106@redhat.com> Date: Fri, 12 Jun 2015 05:41:00 -0000 From: Jason Merrill User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Kai Tietz CC: gcc-patches List Subject: C++ delayed folding branch review Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2015-06/txt/msg00894.txt.bz2 Generally, it seems like most of my comments from April haven't been addressed yet. > @@ -3023,13 +3023,14 @@ conversion_warning (location_t loc, tree type, tree expr 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. > @@ -589,9 +589,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)); Again, calling fold here is wrong; it doesn't handle constexpr, and we should have folded before we got here. > @@ -5090,9 +5090,9 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3, > > valid_operands: > result = 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 = fold_if_not_in_template (result); > + result = fold (result); This seems related to your status report note: > 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 modify anything. 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. > @@ -5628,8 +5628,8 @@ build_new_op_1 (location_t loc, enum tree_code code, 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 = convert_like (conv, arg2, complain); > } > @@ -5666,7 +5666,8 @@ build_new_op_1 (location_t loc, enum tree_code code, 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 code, int f > lags, tree arg1, > case NE_EXPR: > if ((code_orig_arg1 == BOOLEAN_TYPE) > ^ (code_orig_arg2 == 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: I still think these fold calls should be cp_fully_fold. > @@ -7382,8 +7383,13 @@ build_over_call (struct z_candidate *cand, int flags, tsu > bst_flags_t complain) > > gcc_assert (j <= nargs); > nargs = j; > + { > + tree *fargs = (!nargs ? argarray : (tree *) alloca (nargs * sizeof (tree))) > ; > + for (j = 0; j < nargs; j++) > + fargs[j] = fold_non_dependent_expr (argarray[j]); 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. > @@ -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. > @@ -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. > @@ -1576,13 +1586,15 @@ cxx_eval_unary_expression (const constexpr_ctx *ctx, tre > e t, > enum tree_code code = TREE_CODE (t); > tree type = TREE_TYPE (t); > r = fold_unary_loc (loc, code, type, arg); > - if (r == NULL_TREE) > + if (r == NULL_TREE || !CONSTANT_CLASS_P (r)) > { > if (arg == orig_arg) > r = t; > else > r = build1_loc (loc, code, type, arg); > } > + else > + r = unify_constant (ctx, r, overflow_p); This still should not be necessary. > @@ -1780,7 +1792,8 @@ cxx_eval_component_reference (const constexpr_ctx *ctx, tree t, > if (field == 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 = bit_position (field); > if (bitpos == start && DECL_SIZE (field) == 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)) == INTEGER_TYPE > && TREE_CODE (value) == INTEGER_CST > && tree_fits_shwi_p (bitpos) Again, this shouldn't be necessary, either; the elements of the CONSTRUCTOR should be fully evaluated already. > @@ -2987,19 +3017,15 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t, > constexpr_ctx new_ctx; > tree r = t; > > - if (t == error_mark_node) > + if (!t || t == error_mark_node) > { > *non_constant_p = 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; You were going to test whether the null check was necessary. What was the result? > 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? > @@ -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 testcase. */ > + 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) != 0) > + { > + *non_constant_p = true; > + break; > + } Why does this happen for static2.C when it doesn't happen on the trunk? I think the bug is somewhere else. > @@ -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? > @@ -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? > +static tree > +cp_fold (tree x, hash_map *fold_hash) Looks like cp_fold still doesn't call maybe_constant_value. > + case CALL_EXPR: > + r = fold (x); > + if (TREE_CODE (r) != CALL_EXPR) > + { > + x = cp_fold (r, fold_hash); > + break; > + } > + { > + int i, m = call_expr_nargs (x); > + for (i = 0; i < m; i++) > + { > + CALL_EXPR_ARG (x, i) = cp_fold (CALL_EXPR_ARG (x, i), fold_hash); > + } > + } > + r = fold (x); > + if (TREE_CODE (r) != CALL_EXPR) > + { > + x = cp_fold (r, fold_hash); > + break; > + } > + return org_x; Why return org_x here? Don't we still want to add this to the hash table? 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. > @@ -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? > @@ -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) == type) > return expr; > > - result = cp_convert (type, expr, complain); > + result = ret = cp_convert (type, expr, complain); > > if ((complain & tf_warning) > && c_inhibit_evaluation_warnings == 0) > @@ -646,6 +650,7 @@ cp_convert_and_check (tree type, tree expr, tsubst_flags_t complain) > tree stripped = folded; > tree folded_result > = folded != expr ? cp_convert (type, folded, complain) : result; > + folded_result = 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; > } Again, there seems to be no reason to introduce the new "ret" variable. > @@ -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. > @@ -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. > @@ -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. > @@ -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 = compute_array_index_type (name, size, tf_warning_or_error); > + { > + size = cp_try_fold_to_constant (size); > + itype = compute_array_index_type (name, size, tf_warning_or_error); > + } There's no need to fold here and inside compute_array_index_type. > @@ -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. > @@ -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? > @@ -779,7 +779,8 @@ perform_member_init (tree member, tree init) > in that case. */ > init = build_x_compound_expr_from_list (init, ELK_MEM_INIT, > tf_warning_or_error); > - > + if (init) > + init = fold (init); Again, why fold here, rather than later when something really wants a constant? If that ever actually occurs? > @@ -417,7 +417,9 @@ strip_using_decl (tree decl) > if (decl == NULL_TREE) > return NULL_TREE; > > - while (TREE_CODE (decl) == USING_DECL && !DECL_DEPENDENT_P (decl)) > + while (TREE_CODE (decl) == USING_DECL > + && !DECL_DEPENDENT_P (decl) > + && !uses_template_parms (USING_DECL_SCOPE (decl))) This seems unrelated to delayed folding. > @@ -6136,6 +6139,10 @@ push_to_top_level (void) > else > need_pop = false; > > + if (scope_chain) > + fm = scope_chain->fold_map; > + else > + fm = 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) = 0; > > + if (!fm) > + fm = new hash_map; Why are these hunks so far apart? I would expect them to be all together. > @@ -6473,7 +6473,8 @@ cp_parser_array_notation (location_t loc, cp_parser *parser, tree *init_index, > 2. ARRAY [ EXP : EXP ] > 3. ARRAY [ EXP : EXP : EXP ] */ > > - *init_index = cp_parser_expression (parser); > + *init_index = cp_parser_expression (parser); > + *init_index = cp_try_fold_to_constant (*init_index); > if (cp_lexer_peek_token (parser->lexer)->type != CPP_COLON) > { > /* This indicates that we have a normal array expression. */ > @@ -6484,10 +6485,12 @@ cp_parser_array_notation (location_t loc, cp_parser *parser, tree *init_index, > /* Consume the ':'. */ > cp_lexer_consume_token (parser->lexer); > length_index = cp_parser_expression (parser); > + length_index = cp_try_fold_to_constant (length_index); > if (cp_lexer_peek_token (parser->lexer)->type == CPP_COLON) > { > cp_lexer_consume_token (parser->lexer); > stride = cp_parser_expression (parser); > + stride = cp_try_fold_to_constant (stride); Again, why fold here, rather than later when something really wants a constant? If that ever actually occurs? > @@ -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. > @@ -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 = 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 = cp_build_c_cast (type, expr, tf_warning_or_error); Huh? build_c_cast just calls cp_build_c_cast. > @@ -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 = cp_parser_constant_expression (parser); > + expr = cp_try_fold_to_constant (expr); > if (check_for_bare_parameter_packs (expr)) > expr = 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 = cp_parser_constant_expression (parser); > + expr_hi = cp_try_fold_to_constant (expr_hi); Again, this seems redundant with the call to cxx_constant_value in case_conversion. > @@ -12210,6 +12224,10 @@ cp_parser_static_assert(cp_parser *parser, bool member_p) > /*allow_non_constant_p=*/true, > /*non_constant_p=*/&dummy); > > + /* Make sure we folded it completely before doing trying to get > + constant value. */ > + condition = fold_non_dependent_expr (condition); Again, this is redundant with the code in finish_static_assert. > @@ -16115,6 +16133,7 @@ cp_parser_enumerator_definition (cp_parser* parser, tree type) > cp_lexer_consume_token (parser->lexer); > /* Parse the value. */ > value = cp_parser_constant_expression (parser); > + value = cp_try_fold_to_constant (value); Again, this is redundant with the code in build_enumerator. > @@ -17702,7 +17721,8 @@ cp_parser_direct_declarator (cp_parser* parser, > constant-expression. */ > if (token->type != CPP_CLOSE_SQUARE) > { > - bool non_constant_p; > + bool non_constant_p = false; This is unnecessary, as cp_parser_constant_expression will set non_constant_p. > @@ -19354,12 +19374,10 @@ cp_parser_initializer_clause (cp_parser* parser, 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 > - = cp_parser_constant_expression (parser, > - /*allow_non_constant_p=*/true, > - non_constant_p); > - } > + initializer > + = cp_parser_constant_expression (parser, > + /*allow_non_constant_p=*/true, > + non_constant_p); Let's not reformat unrelated code in a big project like this. > @@ -20955,9 +20973,11 @@ cp_parser_member_declaration (cp_parser* parser) > > /* Consume the `:' token. */ > cp_lexer_consume_token (parser->lexer); > + > /* Get the width of the bitfield. */ > width > = cp_parser_constant_expression (parser); > + width = maybe_constant_value (width); Again, this seems redundant with the call to cxx_constant_value in check_bitfield_decl. 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. > 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; > + > + STRIP_NOPS (expr_ovl); > + switch (code) > + { > + case ABS_EXPR: > + case NEGATE_EXPR: > + if (TREE_CODE (expr) == INTEGER_CST > + || TREE_CODE (expr) == REAL_CST > + || TREE_CODE (expr) == VECTOR_CST > + || TREE_CODE (expr) == FIXED_CST > + || TREE_CODE (expr) == COMPLEX_CST) > + result_ovl = fold (result); > + break; > + default: > + break; > + } Not cp_fully_fold? > @@ -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) == PREINCREMENT_EXPR > - || TREE_CODE (incr) == POSTINCREMENT_EXPR) > + iter_incr = fold (iter_incr); > + if (TREE_CODE (incr) == PREINCREMENT_EXPR > + || TREE_CODE (incr) == 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 = fold (iter_incr); > iter_incr = 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 = TREE_OPERAND (rhs, 0); > + incr = fold (incr); Why are these folds needed? > @@ -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? > @@ -3664,6 +3660,10 @@ convert_arguments (tree typelist, vec **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? > @@ -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 afterward. */ > - tree xop0 = op0, xop1 = op1, xresult_type = result_type; > + tree xop0 = fold (op0), xop1 = fold (op1), xresult_type = result_type; Again, this seems wrong. In fact, the whole short_compare business seems like the sort of early folding we want to do away with. > @@ -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? > @@ -7989,7 +7984,6 @@ expand_ptrmemfunc_cst (tree cst, tree *delta, tree *pfn) > tree binfo = binfo_or_else (orig_class, fn_class); > *delta = build2 (PLUS_EXPR, TREE_TYPE (*delta), > *delta, BINFO_OFFSET (binfo)); > - *delta = 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, tree *pfn) > *pfn = DECL_VINDEX (fn); > *pfn = build2 (MULT_EXPR, integer_type_node, *pfn, > TYPE_SIZE_UNIT (vtable_entry_type)); > - *pfn = fold_if_not_in_template (*pfn); > > switch (TARGET_PTRMEMFUNC_VBIT_LOCATION) > { > case ptrmemfunc_vbit_in_pfn: > *pfn = build2 (PLUS_EXPR, integer_type_node, *pfn, > integer_one_node); > - *pfn = fold_if_not_in_template (*pfn); > break; > > case ptrmemfunc_vbit_in_delta: > *delta = build2 (LSHIFT_EXPR, TREE_TYPE (*delta), > *delta, integer_one_node); > - *delta = fold_if_not_in_template (*delta); > *delta = build2 (PLUS_EXPR, TREE_TYPE (*delta), > *delta, integer_one_node); > - *delta = fold_if_not_in_template (*delta); > break; > > default: > @@ -8021,7 +8011,6 @@ expand_ptrmemfunc_cst (tree cst, tree *delta, tree *pfn) > } > > *pfn = build_nop (TYPE_PTRMEMFUNC_FN_TYPE (type), *pfn); > - *pfn = fold_if_not_in_template (*pfn); 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. > @@ -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 == DWARF2_ADDR_SIZE); > + gcc_assert (val1->v.val_unsigned > + == (unsigned HOST_WIDE_INT) DWARF2_ADDR_SIZE); Again, we need to fix this warning so this change is unnecessary. > @@ -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? > @@ -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? > @@ -1796,6 +1796,9 @@ evaluate_stmt (gimple stmt) > && (likelyvalue == CONSTANT || is_gimple_call (stmt) > || (gimple_assign_single_p (stmt) > && gimple_assign_rhs_code (stmt) == ADDR_EXPR)) > + && (likelyvalue == CONSTANT || is_gimple_call (stmt) > + || (gimple_assign_single_p (stmt) > + && gimple_assign_rhs_code (stmt) == ADDR_EXPR)) You were going to revert this merge error? > @@ -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. > @@ -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. Jason