Hello Jason,
Thanks for the review. I addressed a lot of your comments directly on
svn-branch. See revision r224439.
----- Ursprüngliche Mail -----
> Generally, it seems like most of my comments from April haven't been
> addressed yet.
Yes, most of them.
> > @@ -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.
I looked for dependencies, and address this.
> > @@ -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.
Agreed. I will commit change for this.
Nevertheless CONSTRUCTOR_ELT's value might still be prefixed by nops due
possible overflows, or by negative sign/invert/etc. This is caused by
non-constant-folding of values. I removed for now those folds, as I agree we
should address this at place of value-assignment. Nevertheless we still have
this issue
> > @@ -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.
Issue is that we otherwise detect in conditions that expressions aren't
constant due never-executed-code-path. The diagnostics we can deal
differently, 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
> > @@ -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.
Ok, modified here use of fold to 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.
Hmm, maybe we should limit this folding on constants. So cp_fold_to_constant
()?
> > @@ -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 ...
> > @@ -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.
> > @@ -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.
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 right that
arg is anyway resolved already, and so this might not happen. Nevertheless we
might need to handle here OVERFLOW?
> > @@ -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.
Evaluated yes, but not necessarily folded to constant. Maybe
cp_fold_to_constant could be better choice here?
> > @@ -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?
I will need to redo this test if the boostrap-issue about real.h is resolved.
But I agree that this check could be superflous.
> > 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).
> > @@ -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.
I will try to investigate in more detail, when bootstrap is working again.
> > @@ -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.
> > @@ -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.
> > +static tree
> > +cp_fold (tree x, hash_map<tree, tree> *fold_hash)
>
> 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, isn't
it?
> > + 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?
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 cache it.
But well, we might want to hash here org_x itself ...
> 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 use 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 folding
before gimplification, where these modifications weren't problematic at all.
So we might want to avoid such argument-conversion in context of
cp_try_fold_to_constant at all?
> > @@ -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 ...
> > @@ -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.
Yes, removed it.
I am trying right now a version using here cp_fully_fold instead of fold.
> > @@ -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.
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 right.
> > @@ -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 ...
> > @@ -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 ...
> > @@ -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.
Yeahm within compute_array_index_type should be enough.
> > @@ -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 ...
> > @@ -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.
I will recheck this what example we have for this when bootstrap is working
again.
> > @@ -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?
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.
> > @@ -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.
Yeah, I will remove from branch. Should be nevertheless something pretty
opaque ...
> > @@ -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<tree, tree>;
>
> 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 with
the other if initializes, and doing then later near assignment the optional
allocation.
I can change this, if you prefer.
> > @@ -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?
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, as we
have to fold them early anyway.
> > @@ -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.
> > @@ -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.
Yes, as we don't want to do cast-folding too early.
> > @@ -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.
Well, but we want to do here check_for_bare_parameter_packs directly after
that, and so we need folded value early.
> > @@ -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.
Hmm, yes. We check that.
> > @@ -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.
Hmm, don't see here redundancy. I just see that we need folded-value for leter
call of check_for_bare_parameter_packs. But might not see the obvious
> > @@ -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.
Ok.
> > @@ -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.
Well, but it violates coding-rules. anyway agreed ;)
> > @@ -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.
Ah, this escaped. This should be a cp_try_fold_to_constant.
> 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 to
deal anyway with constant-values. At some please we use those values already
for some diagnostics, so I thoght it is more consistent to do this folding to
constant directly after parsing it. To delay that into builder-routines is IMO
just less clear, and could lead to double-doing foldings. Additionally 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 places
constants aren't needed directly after parsing it those calls.
> > 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?
Hmm, yeah, I changed the switch to a call to cp_try_fold_to_constant, which
should be pretty exactly what we are looking here for diagnostics.
> > @@ -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?
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.
> > @@ -3664,6 +3660,10 @@ convert_arguments (tree typelist, vec<tree, 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. Otherwise the
produce either ICE, or other funny things. We could call here instead also the
cp_try_fold... function.
Btw some lines above there is a comment about NOP_EXPR being extended to
indicate 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.
> > @@ -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.
Ok, I will check if *delta always has a valid type. As otherwise fold_build2
will crash.
> > @@ -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.
Yeah, just something intermediate. It is on my list.
> > @@ -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. 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.
> > @@ -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?
Sure.
> > @@ -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.
> > @@ -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.
> Jason
>
Kai
