Hello Jason,
thanks for your review. I addressed most of your mentioned issues
already today. To some of them I have further comments ...
2015-09-17 8:18 GMT+02:00 Jason Merrill <ja...@redhat.com>:
>> @@ -216,6 +216,7 @@ libgcov-driver-tool.o-warn = -Wno-error
>> libgcov-merge-tool.o-warn = -Wno-error
>> gimple-match.o-warn = -Wno-unused
>> generic-match.o-warn = -Wno-unused
>> +insn-modes.o-warn = -Wno-error
>
>
> This doesn't seem to be needed anymore.
Yes, removed.
>> @@ -397,11 +397,13 @@ convert_to_real (tree type, tree expr)
>> EXPR must be pointer, integer, discrete (enum, char, or bool), float,
>> fixed-point or vector; in other cases error is called.
>>
>> + If DOFOLD is TRZE, we try to simplify newly-created patterns by
>> folding.
>
>
> "TRUE"
Fixed. Thanks.
>> + if (!dofold)
>> + {
>> + expr = build1 (CONVERT_EXPR,
>> + lang_hooks.types.type_for_size
>> + (TYPE_PRECISION (intype), 0),
>> + expr);
>> + return build1 (CONVERT_EXPR, type, expr);
>> + }
>
>
> When we're not folding, I don't think we want to do the two-step conversion,
> just the second one. And we might want to use NOP_EXPR instead of
> CONVERT_EXPR, but I'm not sure about that.
This is indeed a point I was thinking about while writing this code.
The comments below regarding other introduced patterns are related to
the same thinking.
Sure is that we want to remove such code-modification from FEs itself,
but that means that those conversions need to be handled elsewhere
later in ME. We have a different functional change about
shorten_compare already, which makes me headaches, as we don't deal
with all cases of it in ME right (which is a different task not
directly related to delayed-folding, but linked). Therefore I kept
those code-modification-code also for df, which we should move out
into ME (with other patterns here in convert.c) in different task.
For above code we need to do 2 step folding, as we need to make sure
that we convert expr first to language-specific size-type, which isn't
necessarily the same as TYPE here. Casting directly to TYPE might
cause side-effects (especially sign/unsigned-conversion issues I could
think about here)
>> @@ -818,10 +828,15 @@ convert_to_integer (tree type, tree expr)
>> if (TYPE_UNSIGNED (typex))
>> typex = signed_type_for (typex);
>> }
>> - return convert (type,
>> - fold_build2 (ex_form, typex,
>> - convert (typex, arg0),
>> - convert (typex, arg1)));
>> + if (dofold)
>> + return convert (type,
>> + fold_build2 (ex_form, typex,
>> + convert (typex, arg0),
>> + convert (typex,
>> arg1)));
>> + arg0 = build1 (CONVERT_EXPR, typex, arg0);
>> + arg1 = build1 (CONVERT_EXPR, typex, arg1);
>> + expr = build2 (ex_form, typex, arg0, arg1);
>> + return build1 (CONVERT_EXPR, type, expr);
>
>
> This code path seems to be for pushing a conversion down into a binary
> expression. We shouldn't do this at all when we aren't folding.
Yes, but see comment above. I agree that such code-modifications are
not the thing we intend to do, but this should be part of a different
task.
>> @@ -845,9 +860,14 @@ convert_to_integer (tree type, tree expr)
>>
>> if (!TYPE_UNSIGNED (typex))
>> typex = unsigned_type_for (typex);
>> + if (!dofold)
>> + return build1 (CONVERT_EXPR, type,
>> + build1 (ex_form, typex,
>> + build1 (CONVERT_EXPR, typex,
>> + TREE_OPERAND (expr, 0))));
>
>
> Likewise.
^^
>> @@ -867,6 +887,14 @@ convert_to_integer (tree type, tree expr)
>> the conditional and never loses. A COND_EXPR may have a
>> throw
>> as one operand, which then has void type. Just leave void
>> operands as they are. */
>> + if (!dofold)
>> + return build3 (COND_EXPR, type, TREE_OPERAND (expr, 0),
>> + VOID_TYPE_P (TREE_TYPE (TREE_OPERAND (expr,
>> 1)))
>> + ? TREE_OPERAND (expr, 1)
>> + : build1 (CONVERT_EXPR, type, TREE_OPERAND
>> (expr, 1)),
>> + VOID_TYPE_P (TREE_TYPE (TREE_OPERAND (expr,
>> 2)))
>> + ? TREE_OPERAND (expr, 2)
>> + : build1 (CONVERT_EXPR, type, TREE_OPERAND
>> (expr, 2)));
>
>
> Likewise.
^^
>> @@ -903,6 +933,10 @@ convert_to_integer (tree type, tree expr)
>> return build1 (FIXED_CONVERT_EXPR, type, expr);
>>
>> case COMPLEX_TYPE:
>> + if (!dofold)
>> + return build1 (CONVERT_EXPR, type,
>> + build1 (REALPART_EXPR,
>> + TREE_TYPE (TREE_TYPE (expr)), expr));
>
>
> Why can't we call convert here rather than build1 a CONVERT_EXPR?
The point about calling convert is that it is overloaded for C++ and
does too much. Additionally, as this function gets called by convert
() indirectly, we could end-up in end-less recursion.
> It would be good to ask a fold/convert maintainer to review the changes to
> this file, too.
Sure.
>> @@ -5671,8 +5668,8 @@ build_new_op_1 (location_t loc, enum tree_code code,
>> int flags, 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);
>> }
>> @@ -5710,7 +5707,7 @@ build_new_op_1 (location_t loc, enum tree_code code,
>> int flags, tree arg1,
>> case TRUTH_OR_EXPR:
>> 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));
>> /* Fall through. */
>> case GT_EXPR:
>> case LT_EXPR:
>> @@ -5721,9 +5718,10 @@ build_new_op_1 (location_t loc, enum tree_code
>> code, int flags, tree arg1,
>> if ((complain & tf_warning)
>> && ((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));
>> if (complain & tf_warning && warn_tautological_compare)
>> - warn_tautological_cmp (loc, code, arg1, arg2);
>> + warn_tautological_cmp (loc, code, fold (arg1), fold (arg2));
>
>
> Would it make sense to change all these folds to maybe_constant_value?
Hmm, maybe_constant_value does IMO too much. Actually we don't want
to in general to resolve here constexpressions, aren't we?
Additionally I think that overhead and costs of maybe_constant_value
is much higher then the fold call.
If you think we want to resolve more complex-expressions and constexpr
to a single-constant, then I am fine to change those uses to
maybe_constant_value instead.
>> @@ -7433,8 +7431,13 @@ build_over_call (struct z_candidate *cand, int
>> flags, tsubst_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]);
>>
>> - check_function_arguments (TREE_TYPE (fn), nargs, argarray);
>> + check_function_arguments (TREE_TYPE (fn), nargs, fargs);
>> + }
>
>
> Using fold_non_dependent_expr is wrong here; we can't be in a template at
> this point, so it is equivalent to maybe_constant_value. We might also
> avoid folding entirely if !warn_nonnull.
Yes, we check in template-declaration-case on top of this function.
So I change this to maybe_constant_value.
I think we will need to check for warn_format, too. I will commit a
optimization that we avoid doing all those preparations only if
warn_nonnull or warn_format is enabled.
>> @@ -428,7 +428,7 @@ build_base_path (enum tree_code code,
>>
>> t = TREE_TYPE (TYPE_VFIELD (current_class_type));
>> t = build_pointer_type (t);
>> - v_offset = convert (t, current_vtt_parm);
>> + v_offset = fold (convert (t, current_vtt_parm));
>
>
> This case should be fine with fold_convert: we're changing from one pointer
> type to another.
Well, I might was here too conservative. I had at different point a
case of convert invocation where fold_convert was actually not doing
the right that due it is the C-cast variant only.
I rechecked, and fold_convert is here indeed fine too. Committed.
>> @@ -2538,12 +2551,19 @@ cxx_eval_indirect_ref (const constexpr_ctx *ctx,
>> tree t,
>> bool lval,
>> bool *non_constant_p, bool *overflow_p)
>> {
>> - tree orig_op0 = TREE_OPERAND (t, 0);
>> + tree r, orig_op0 = TREE_OPERAND (t, 0);
>> bool empty_base = false;
>>
>> + if (TREE_CODE (t) == MEM_REF
>> + && (!TREE_OPERAND (t, 1) || !integer_zerop (TREE_OPERAND (t, 1))))
>> + {
>> + *non_constant_p = true;
>> + return t;
>> + }
>> +
>> /* First try to simplify it directly. */
>> - tree r = cxx_fold_indirect_ref (EXPR_LOCATION (t), TREE_TYPE (t),
>> orig_op0,
>> - &empty_base);
>> + r = cxx_fold_indirect_ref (EXPR_LOCATION (t), TREE_TYPE (t), orig_op0,
>> + &empty_ba
>
>
> I don't see a reason for the change to where 'r' is declared.
Yes, I reverted this. My idea was here to shorten the
cxx_fold_indirect_ref... line a bit.
> Please add a comment about MEM_REF.
Done.
>> @@ -3347,6 +3372,8 @@ cxx_eval_constant_expression (const constexpr_ctx
>> *ctx, tree t,
>> /* Don't VERIFY_CONSTANT here. */
>> if (*non_constant_p)
>> return t;
>> + if (TREE_CODE (op) == CONSTRUCTOR)
>> + return t;
>> gcc_checking_assert (TREE_CODE (op) != CONSTRUCTOR);
>
>
> This is wrong.
The check for CONSTRUCTOR is superflous. I checked. removed code-change.
>> @@ -3370,6 +3397,10 @@ cxx_eval_constant_expression (const constexpr_ctx
>> *ctx, tree t,
>> break;
>>
>> case SIZEOF_EXPR:
>> + if (processing_template_decl
>> + && (!COMPLETE_TYPE_P (TREE_TYPE (t))
>> + || TREE_CODE (TYPE_SIZE (TREE_TYPE (t))) != INTEGER_CST))
>> + return t;
>
>
> This is still wrong.
Current code tries to allow resolving of sizeof_expr on complete,
none-vla types within templates. You mean we should reject all cases
if processing_template_decl is true?
>> @@ -3651,6 +3689,9 @@ cxx_eval_constant_expression (const constexpr_ctx
>> *ctx, tree t,
>> non_constant_p, overflow_p, jump_target);
>> break;
>>
>> + case ARRAY_NOTATION_REF:
>> + break;
>
>
> This seems unnecessary; an ARRAY_NOTATION_REF shouldn't get this far.
We see it. This is related to use of maybe_constant_value in much
more places, like finish_unary_op_expr, cp_convert_and_check, etc.
So, no, we need it.
>> +/* Helper routine for fold_simplet function. Either return simplified
>
>
> s/simplet/simple/
Fixed, thanks.
>> +static tree
>> +cp_fold_r (tree *stmt_p, int *walk_subtrees ATTRIBUTE_UNUSED, void
>> *data)
>
>
> walk_subtrees is used now.
Adjusted, thanks.
> Also, this function needs more comments.
I added a function comment. I added there some lines about the
purpose, and that we want to move folding of common expressions out of
the FE to the ME.
>> +static tree
>> +cp_fold (tree x, hash_map<tree, tree> *fold_hash)
>
>
> This also needs a lot more comments.
I added function-comment.
Side-note: by looking on initial checks for processing_template_decl,
or valid fold_hash, I think we can move that out to users of cp_fold
instead. By this we avoid redundant checks of them. This might not
much, but could be worth.
>> @@ -642,13 +645,14 @@ cp_convert_and_check (tree type, tree expr,
>> tsubst_flags_t
>> complain)
>> {
>> tree folded = maybe_constant_value (expr);
>> tree stripped = folded;
>> - tree folded_result
>> + tree folded_result;
>> + folded_result
>> = folded != expr ? cp_convert (type, folded, complain) : result;
>> -
>> - /* maybe_constant_value wraps an INTEGER_CST with TREE_OVERFLOW in
>> a
>> - NOP_EXPR so that it isn't TREE_CONSTANT anymore. */
>> + folded_result = cp_fully_fold (folded_result);
>> + /* The maybe_constant_value wraps an INTEGER_CST with TREE_OVERFLOW
>> + in a NOP_EXPR so that it isn't TREE_CONSTANT anymore. */
>> STRIP_NOPS (stripped);
>> -
>> + folded = cp_fully_fold (folded);
>
>
> Now that we're caching maybe_constant_value, I think we can replace most of
> the calls to cp_fully_fold with either maybe_constant_value or fold_simple.
Replacing it with fold_simple looks to me not good, as we don't cache
it, and it is likely that we perform same operation many times on one
expression.
The use of maybe_constant_value might be good, but we need to deal
with NOPS again ...
> Here, we already called maybe_constant_value, so there's no need to fold
> folded again. And instead of folding folded_result, we should be able to
> replace the cp_convert with cp_fold_convert.
It is, as we add possibly a convert on the expression, which we want
to be folded away.
>> @@ -3362,6 +3362,11 @@ finish_case_label (location_t loc, tree low_value,
>> tree high_value)
>> {
>> tree label;
>>
>> + /* For build_case_label we need to make sure that arguments
>> + got fully folded. */
>> + low_value = cp_fully_fold (low_value);
>> + high_value = cp_fully_fold (high_value);
>
>
> Looks like you removed another set of folds in this function, but this one
> still needs to go away.
We could use here instead fold_simple, or maybe_constant_value (later
would be wrong IMO due semantics), but we need to make sure we reduce
to constant value. Therefore this folding is in some way necessary
(think about invert, negates, or similiar).
>> @@ -6555,12 +6561,13 @@ cp_finish_decl (tree decl, tree init, bool
>> init_const_expr_p,
>>
>> if (init && VAR_P (decl))
>> {
>> + init_cst = cp_fully_fold (init);
>> DECL_NONTRIVIALLY_INITIALIZED_P (decl) = 1;
>> /* If DECL is a reference, then we want to know whether init is a
>> reference constant; init_const_expr_p as passed tells us whether
>> it's an rvalue constant. */
>> if (TREE_CODE (type) == REFERENCE_TYPE)
>> - init_const_expr_p = potential_constant_expression (init);
>> + init_const_expr_p = potential_constant_expression (init_cst);
>> if (init_const_expr_p)
>> {
>> /* Set these flags now for templates. We'll update the flags in
>> @@ -6622,10 +6629,11 @@ cp_finish_decl (tree decl, tree init, bool
>> init_const_expr_p,
>> && !MAYBE_CLASS_TYPE_P (type))
>> init = build_x_compound_expr_from_list (init, ELK_INIT,
>> tf_warning_or_error);
>> + init_cst = init;
>> }
>>
>> if (init)
>> - DECL_INITIAL (decl) = init;
>> + DECL_INITIAL (decl) = init_cst;
>> return;
>> }
>
>
> We don't want this folding here in cp_finish_decl; we agreed before to fold
> initializers, but I think the right place to do this is where you already
> added it to store_init_value, not here.
I tested it, and I found that we have still cases escaping this. I
will recheck, and will try provide more details for it, but I expect
that it is still necessary.
>> @@ -8674,16 +8653,18 @@ compute_array_index_type (tree name, tree size,
>> tsubst_flags_t complain)
>> SET_TYPE_STRUCTURAL_EQUALITY (itype);
>> return itype;
>> }
>> -
>> +
>> + tree size_constant = cp_fully_fold (size);
>
>
> I've been saying all along that this is redundant.
Due we fold only in case of !processing_template_decl anyway, the only
case where we might not have a constant value is the case that dialect
is < cxx11 and expression is encapsulated in a nop_expr with
side-effects.
I see just this case, where we don't call maybe_constant_value for it.
cp_fully_fold nukes this nop_expr on result, otherwise you should be
right here
>> @@ -3493,6 +3495,8 @@ build_vec_init (tree base, tree maxindex, tree init,
>> }
>>
>> maxindex = cp_convert (ptrdiff_type_node, maxindex, complain);
>> + maxindex = maybe_constant_value (maxindex);
>
>
> I think this can be fold_simple.
Yes, I committed this change.
Side-note: fold_simple isn't caching, so it might be still slower in some cases.
>> @@ -6648,6 +6648,11 @@ 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 need to fold for negative constants so that diagnostic in
>> + c-family/c-common.c doesn't fail for array-bounds. */
>> + index = fold_simple (index);
>
>
> This continues to be the wrong place for this.
So doing fold_simple on index_exp argument within grok_array_decl ()
is the better place?
If you agree, I will add it there.
>> @@ -8124,7 +8129,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);
>
>
> And this change still has no effect.
I modified it to call build_c_cast instead. I had moved it to
cp_build_c_cast, as this is the underlying function. Additionally it
didn't required the location argument.
>> @@ -8286,9 +8293,11 @@ cp_parser_binary_expression (cp_parser* parser,
>> bool cast
>> _p,
>> /* For "false && x" or "true || x", x will never be executed;
>> disable warnings while evaluating it. */
>> if (current.tree_type == TRUTH_ANDIF_EXPR)
>> - c_inhibit_evaluation_warnings += current.lhs ==
>> truthvalue_false_node;
>> + c_inhibit_evaluation_warnings +=
>> + cp_fully_fold (current.lhs) == truthvalue_false_node;
>> else if (current.tree_type == TRUTH_ORIF_EXPR)
>> - c_inhibit_evaluation_warnings += current.lhs ==
>> truthvalue_true_node;
>> + c_inhibit_evaluation_warnings +=
>> + cp_fully_fold (current.lhs) == truthvalue_true_node;
>>
>> /* Extract another operand. It may be the RHS of this expression
>> or the LHS of a new, higher priority expression. */
>> @@ -8332,9 +8341,11 @@ cp_parser_binary_expression (cp_parser* parser,
>> bool cast_p,
>>
>> /* Undo the disabling of warnings done above. */
>> if (current.tree_type == TRUTH_ANDIF_EXPR)
>> - c_inhibit_evaluation_warnings -= current.lhs ==
>> truthvalue_false_node;
>> + c_inhibit_evaluation_warnings -=
>> + cp_fully_fold (current.lhs) == truthvalue_false_node;
>> else if (current.tree_type == TRUTH_ORIF_EXPR)
>> - c_inhibit_evaluation_warnings -= current.lhs ==
>> truthvalue_true_node;
>> + c_inhibit_evaluation_warnings -=
>> + cp_fully_fold (current.lhs) == truthvalue_true_node;
>>
>> if (warn_logical_not_paren
>> && TREE_CODE_CLASS (current.tree_type) == tcc_comparison
>> @@ -8421,7 +8432,7 @@ cp_parser_binary_expression (cp_parser* parser, bool
>> cast_p,
>> static tree
>> cp_parser_question_colon_clause (cp_parser* parser, tree logical_or_expr)
>> {
>> - tree expr;
>> {
>> - tree expr;
>> + tree expr, folded_logical_or_expr = cp_fully_fold (logical_or_expr);
>> tree assignment_expr;
>> struct cp_token *token;
>> location_t loc = cp_lexer_peek_token (parser->lexer)->location;
>> @@ -8436,7 +8447,8 @@ cp_parser_question_colon_clause (cp_parser* parser,
>> tree logical_or_expr)
>> "ISO C++ does not allow ?: with omitted middle operand");
>> /* Implicit true clause. */
>> expr = NULL_TREE;
>> - c_inhibit_evaluation_warnings += logical_or_expr ==
>> truthvalue_true_node;
>> - c_inhibit_evaluation_warnings += current.lhs ==
>> truthvalue_true_node;
>> + c_inhibit_evaluation_warnings +=
>> + cp_fully_fold (current.lhs) == truthvalue_true_node;
>>
>> /* Extract another operand. It may be the RHS of this expression
>> or the LHS of a new, higher priority expression. */
>> @@ -8332,9 +8341,11 @@ cp_parser_binary_expression (cp_parser* parser,
>> bool cast
>> _p,
>>
>> /* Undo the disabling of warnings done above. */
>> if (current.tree_type == TRUTH_ANDIF_EXPR)
>> - c_inhibit_evaluation_warnings -= current.lhs ==
>> truthvalue_false_node;
>> + c_inhibit_evaluation_warnings -=
>> + cp_fully_fold (current.lhs) == truthvalue_false_node;
>> else if (current.tree_type == TRUTH_ORIF_EXPR)
>> - c_inhibit_evaluation_warnings -= current.lhs ==
>> truthvalue_true_node;
>> + c_inhibit_evaluation_warnings -=
>> + cp_fully_fold (current.lhs) == truthvalue_true_node;
>>
>> if (warn_logical_not_paren
>> && TREE_CODE_CLASS (current.tree_type) == tcc_comparison
>> @@ -8421,7 +8432,7 @@ cp_parser_binary_expression (cp_parser* parser, bool
>> cast_
>> p,
>> static tree
>> cp_parser_question_colon_clause (cp_parser* parser, tree logical_or_expr)
>> {
>> - tree expr;
>> + tree expr, folded_logical_or_expr = cp_fully_fold (logical_or_expr);
>
>
> These should change to maybe_constant_value.
I will do check, and come back on this later.
>> @@ -12311,6 +12325,11 @@ cp_parser_static_assert(cp_parser *parser, bool
>> member_p)
>> /*allow_non_constant_p=*/true,
>> /*non_constant_p=*/&dummy);
>>
>> + /* Reduce condition early. We need to reduce builtins within
>> + static_asserts, so that testcase like pr62024.C getting
>> + resolved always proper. */
>> + condition = fold_non_dependent_expr (condition);
>
>
> If pr62024.C isn't handled properly by maybe_constant_value, that should be
> fixed rather than worked around here.
Will check on this later too. The maybe_constant_value should be ok for this.
>> @@ -28920,6 +28940,11 @@ cp_parser_omp_var_list_no_open (cp_parser
>> *parser, enum omp_clause_code kind,
>> CPP_CLOSE_SQUARE))
>> length = cp_parser_expression (parser);
>> }
>> + /* So we need here fully folded values. */
>> + if (length)
>> + length = cp_fully_fold (length);
>> + if (low_bound)
>> + low_bound = cp_fully_fold (low_bound);
>
>
> I think these can change to maybe_constant_value.
Ok, changed.
>> @@ -31276,6 +31302,8 @@ cp_parser_omp_for_incr (cp_parser *parser, tree
>> decl)
>> else
>> lhs = build_x_unary_op (input_location, NEGATE_EXPR, rhs,
>> tf_warning_or_error);
>> + if (op != PLUS_EXPR && CONSTANT_CLASS_P (rhs))
>> + lhs = fold (lhs);
>
>
> This should be fold_simple.
Ok., changed.
>> @@ -29856,6 +29881,7 @@ cp_parser_omp_clause_aligned (cp_parser *parser,
>> tree li
>> st)
>> if (colon)
>> {
>> alignment = cp_parser_constant_expression (parser);
>> + alignment = alignment;
>
>
> This seems unnecessary. :)
Heh, indeed yes.
>> @@ -4616,10 +4612,12 @@ cp_build_binary_op (location_t location,
>> op1 = save_expr (op1);
>>
>> pfn0 = pfn_from_ptrmemfunc (op0);
>> + pfn0 = cp_fully_fold (pfn0);
>> /* Avoid -Waddress warnings (c++/64877). */
>> if (TREE_CODE (pfn0) == ADDR_EXPR)
>> TREE_NO_WARNING (pfn0) = 1;
>> pfn1 = pfn_from_ptrmemfunc (op1);
>> + pfn1 = cp_fully_fold (pfn1);
>
>
> I'm not sure what the fold here is for. Your checkin message suggests that
> it's there to expose the ADDR_EXPR so we can set TREE_NO_WARNING on it. I
> think it would be better to set TREE_NO_WARNING when we call build_addr_func
> in expand_ptrmemfunc_cst rather than here.
AFAIR it is indeed about ADDR_EXPR. I had tested your suggested
variant already, and it didn't worked as intended AFAIR. I will come
back to it.
>> @@ -4992,9 +4992,9 @@ cp_build_binary_op (location_t location,
>> tree oop1 = maybe_constant_value (orig_op1);
>>
>> if (TREE_CODE (oop0) != INTEGER_CST)
>> - oop0 = orig_op0;
>> + oop0 = cp_fully_fold (orig_op0);
>> if (TREE_CODE (oop1) != INTEGER_CST)
>> - oop1 = orig_op1;
>> + oop1 = cp_fully_fold (orig_op1);
>
>
> If we're going to need cp_fully_fold here, we should call it instead of
> maybe_constant_value rather than call both functions.
Hmm, but maybe_constant_value resolves possible constexpr ... For oop1
we might not need cp_fully_fold, but for oop0 we seems to need it.
>> @@ -882,6 +884,7 @@ check_narrowing (tree type, tree init, tsubst_flags_t
>> complain)
>> }
>>
>> init = fold_non_dependent_expr (init);
>> + init = cp_fully_fold (init);
>
>
> What is this for?
I will recheck, but AFAIR it is required to resolve some expressions
like pointer-difference, offsetof-stuff, etc.
>> @@ -1859,7 +1859,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);
>
>
> This shouldn't be necessary anymore.
Yes, this escaped. Reverted change.
>> @@ -7294,7 +7294,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);
>
>
> Nor this, now that we're folding the OMP condition.
No, this change is still required. Testcases
c-c++-common/clik-plus/PS/vectorlength.c, and
c-c++-common/clik-plus/PS/for1.C are failing with that assert.
It is true that we handle OMP folding right, but this means especially
that we don't fold conditon itself, just its arguments.
Issue is that we see on both sides of this OMP_FOR_COND a var_decl.
Sadly the second is that one we are interested in ...
For vectorlength.c testcase we see the following AST in dump:
<gt_expr 0xffb92400
type <boolean_type 0xffcd0720 bool public unsigned QI
size <integer_cst 0xffde0ea0 constant 8>
unit size <integer_cst 0xffde0eb8 constant 1>
align 8 symtab 0 alias set -1 canonical type 0xffcd0720
precision 1 min <integer_cst 0xffde1110 0> max <integer_cst 0xffde1140
1>>
side-effects
arg 0 <cleanup_point_expr 0xffb92178
type <integer_type 0xffcd0420 int sizes-gimplified public SI
size <integer_cst 0xffde0ff0 constant 32>
unit size <integer_cst 0xffde1008 constant 4>
align 32 symtab 0 alias set -1 canonical type 0xffcd0420
precision 32 min <integer_cst 0xffde0fa8 -2147483648> max <integer_cst
0xffde0fc0 2147483647>
pointer_to_this <pointer_type 0xffcd0de0>>
side-effects
arg 0 <nop_expr 0xffb91fb0 type <integer_type 0xffcd0420 int>
side-effects arg 0 <var_decl 0xffda0160 N>>>
arg 1 <var_decl 0xffda01b8 i type <integer_type 0xffcd0420 int>
used tree_1 decl_5 SI file vectorlength.c line 15 col 12 size
<integer_cst 0xffde0ff0 32> unit size <integer_cst 0xffde1008 4>
align 32 context <function_decl 0xffd76800 foo>>>
I assume it gets sorted right due side-effects of other var_decl.
>> @@ -7872,8 +7872,7 @@ get_delta_difference (tree from, tree to,
>> }
>> }
>>
>> - return fold_if_not_in_template (convert_to_integer (ptrdiff_type_node,
>> - result));
>> + return convert_to_integer (ptrdiff_type_node, result);
>> }
>>
>> /* Return a constructor for the pointer-to-member-function TYPE using
>> @@ -8056,7 +8055,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
>> @@ -8064,23 +8062,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:
>> @@ -8088,7 +8082,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);
>
>
> I still think we want to keep these folds, since the trees are all compiler
> generated.
So, we it just for constants, aren't we? So we could invoke
fold_simple for it? I would assume that fold has higher costs in
average.
Disadvantage of fold_simple over cp_fully_fold is that we don't cache
on it. Not sure if this matters here.
>> @@ -1859,7 +1859,8 @@ output_loc_operands (dw_loc_descr_ref loc, int
>> for_eh_or_s
>> kip)
>> }
>> 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);
>
>
> This isn't needed anymore.
Seems to be mentioned twice ... see above.
>> @@ -7294,7 +7294,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);
>
>
> And this shouldn't be, either.
Likewise, is mentioned second time. See above for comment.
> Jason
Kai
