public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* C++ delayed folding branch review
@ 2015-06-12  5:41 Jason Merrill
  2015-06-12 16:17 ` Kai Tietz
  0 siblings, 1 reply; 38+ messages in thread
From: Jason Merrill @ 2015-06-12  5:41 UTC (permalink / raw)
  To: Kai Tietz; +Cc: gcc-patches List

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<tree, tree> *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<tree, tree>;

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<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?

> @@ -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

^ permalink raw reply	[flat|nested] 38+ messages in thread
* C++ delayed folding branch review
@ 2015-04-24  4:23 Jason Merrill
  2015-04-24 13:46 ` Kai Tietz
  0 siblings, 1 reply; 38+ messages in thread
From: Jason Merrill @ 2015-04-24  4:23 UTC (permalink / raw)
  To: Kai Tietz; +Cc: gcc-patches List

> +  expr = fold (expr);
>    /* This may happen, because for LHS op= RHS we preevaluate
>       RHS and create C_MAYBE_CONST_EXPR <SAVE_EXPR <RHS>>, which
>       means we could no longer see the code of the EXPR.  */
>    if (TREE_CODE (expr) == C_MAYBE_CONST_EXPR)
>      expr = C_MAYBE_CONST_EXPR_EXPR (expr);
>    if (TREE_CODE (expr) == SAVE_EXPR)
> -    expr = TREE_OPERAND (expr, 0);
> +    expr = fold (TREE_OPERAND (expr, 0));

How about moving the first fold after the SAVE_EXPR block, so that we 
only need to call fold once?

> +    case NEGATE_EXPR:
> +    case BIT_NOT_EXPR:
> +    case CONVERT_EXPR:
> +    case VIEW_CONVERT_EXPR:
> +    case NOP_EXPR:
> +    case FIX_TRUNC_EXPR:
> +      {
> +       tree op1 = TREE_OPERAND (expr, 0);
> +       tree fop1 = fold (op1);
> +       if (fop1 && op1 != fop1)
> +         fop1 = fold_build1_loc (loc, TREE_CODE (expr), TREE_TYPE (expr),
> +                                 fop1);

Isn't this redundant with the call to fold above?  If not, it seems that 
the above call should be to *_fully_fold.  I suppose we want an entry 
point defined by both front ends that c-common code can call which does 
full folding of an expression.

> @@ -597,9 +597,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));

Calling fold here is wrong; it doesn't handle constexpr, and we should 
have folded before we got here.

>                 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));

I think warn_logical_operator should call back into *_fully_fold. 
Likewise for most similar added calls to fold.

> @@ -7356,8 +7354,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]);

Similarly, this and build_cxx_call should use cp_fully_fold.

> @@ -7602,7 +7614,6 @@ build_cxx_call (tree fn, int nargs, tree *argarray,
>        && current_function_decl
>        && DECL_DECLARED_CONSTEXPR_P (current_function_decl))
>      optimize = 1;
> -  fn = fold_if_not_in_template (fn);
>    optimize = optimize_sav;

Since we're removing the fold, we can also remove the changes to "optimize".

> @@ -443,7 +443,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));

fold_convert should work here.

> @@ -576,7 +576,6 @@ build_simple_base_path (tree expr, tree binfo)
>         expr = build3 (COMPONENT_REF,
>                        cp_build_qualified_type (type, type_quals),
>                        expr, field, NULL_TREE);
> -       expr = fold_if_not_in_template (expr);

I don't think we need to remove this fold, since it is part of compiler 
internals rather than something the user wrote.  Really, we should 
represent the base conversion with something like a CONVERT_EXPR and 
only call this function when we want to fold it.  But that can wait for 
a later patch.

> @@ -1046,6 +1048,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;
...
>  reduced_constant_expression_p (tree t)
>  {
> +  /* Make sure we remove useless initial NOP_EXPRs.  */
> +  STRIP_NOPS (t);


Where are these NOPs coming from?

> @@ -1082,7 +1087,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 should not be necessary.

> @@ -1765,7 +1780,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);
...
> @@ -1849,7 +1865,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);

This shouldn't be necessary, either; the elements of the CONSTRUCTOR 
should be fully evaluated already.

> @@ -1560,14 +1570,19 @@ cxx_eval_unary_expression (const constexpr_ctx *ctx, tre
> e t,
>    location_t loc = EXPR_LOCATION (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 (TREE_CODE (t) == UNARY_PLUS_EXPR)
> +    r = fold_convert_loc (loc, TREE_TYPE (t), arg);

We don't want to handle UNARY_PLUS_EXPR here; we should handle it like 
NOP_EXPR.  And so you shouldn't need the call to unify_constant.

>      case BIT_NOT_EXPR:
>      case TRUTH_NOT_EXPR:
>      case FIXED_CONVERT_EXPR:
> +    case UNARY_PLUS_EXPR:
>        r = cxx_eval_unary_expression (ctx, t, lval,

So this case should be down with NOP_EXPR.

> @@ -2954,19 +2987,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)

Where are null expressions coming from?

>      case SIZEOF_EXPR:
> +      if (processing_template_decl
> +         && (!COMPLETE_TYPE_P (TREE_TYPE (t))
> +         || TREE_CODE (TYPE_SIZE (TREE_TYPE (t))) != INTEGER_CST))
> +       return t;

The type of a SIZEOF_EXPR will always be size_t, so this isn't actually 
accomplishing anything, and should be removed.

> +      /* 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 can this happen on the branch but not on trunk?  I think the problem 
is elsewhere.

>      case NOP_EXPR:
>        {
>         tree oldop = TREE_OPERAND (t, 0);
> +       if (TREE_CODE (t) == 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;
> +         }

This doesn't seem like the right place to handle this; why didn't we 
diagnose the overflow when it happened?

>  maybe_constant_init (tree t, tree decl)
>  {
> +  if (!t)
> +    return t;

Where are null initializers coming from?

>      case MINUS_EXPR:
>        /* -- a subtraction where both operands are pointers.   */
>        if (TYPE_PTR_P (TREE_OPERAND (t, 0))
> -          && TYPE_PTR_P (TREE_OPERAND (t, 1)))
> +          && TYPE_PTR_P (TREE_OPERAND (t, 1))
> +         && TREE_OPERAND (t, 0) != TREE_OPERAND (t, 1))

Why?  From where are we getting a pointer subtracted from itself?

That said, we should probably just remove this case and the next, as 
they are obsolete.  I'll remove them on the trunk.

> +static tree
> +cp_fold (tree x, hash_map<tree, tree> *fold_hash)
> +{
....

I still think we need a hybrid of this and the constexpr code: it isn't 
full folding if we aren't doing constexpr evaluation.  But we can't just 
use maybe_constant_value because that only folds C++ 
constant-expressions, and we want to fold more things than that.  I 
suppose one simple approach for now would be to call 
maybe_constant_value from cp_fold.

> @@ -614,9 +614,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);

Why?  If we're calling cp_fold_convert in a place where we don't want to 
fold, we should stop calling it rather than change it.

>  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)
> @@ -652,6 +656,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.  */
> @@ -663,7 +668,7 @@ cp_convert_and_check (tree type, tree expr, tsubst_flags_t complain)
>                                         folded_result);
>      }
>
> -  return result;
> +  return ret;

Why introduce the "ret" variable?  It doesn't seem to do anything 
different from "result".  And instead of the added fold, maybe change 
the cp_convert on the previous line to cp_fold_convert?

> @@ -1535,8 +1538,10 @@ build_expr_type_conversion (int desires, tree expr, bool complain)
> +  tree expr_folded = maybe_constant_value (expr);
>
> -  if (expr == null_node
> +  STRIP_NOPS (expr_folded);
> +  if (expr_folded == null_node

We shouldn't need to fold to check for null_node, it only occurs when 
explicitly written.  And 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.

> @@ -8502,16 +8502,18 @@ compute_array_index_type (tree name, tree size, tsubst_flags_t complain)
> +  /* We need to do fully folding to determine if we have VLA, or not.  */
> +  tree size_constant = maybe_constant_value (size);

Why is this needed?  We already call maybe_constant_value earlier in 
compute_array_index_type.

> -      itype = fold (itype);
> +      itype = maybe_constant_value (itype);
> -               itype = variable_size (fold (newitype));
> +               itype = variable_size (maybe_constant_value (newitype));

Maybe these should use cp_fully_fold?

> @@ -13090,6 +13092,8 @@ build_enumerator (tree name, tree value, tree enumtype, location_t loc)
> +  if (value)
> +    value = maybe_constant_value (value);

This seems unnecessary, since we call cxx_constant_value below.

>               value = cxx_constant_value (value);
> +             STRIP_NOPS (value);

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?

> -          value = convert (ENUM_UNDERLYING_TYPE (enumtype), value);
> +          value = fold (convert (ENUM_UNDERLYING_TYPE (enumtype), value));

fold_convert again.

> @@ -188,9 +188,9 @@ build_zero_init_1 (tree type, tree nelts, bool static_storage_p,
> -    init = convert (type, nullptr_node);
> +    init = fold (convert (type, nullptr_node));

fold_convert

> @@ -783,7 +783,8 @@ perform_member_init (tree member, tree init)
> +      if (init)
> +       init = fold (init);

Why fold here?  This doesn't seem like a place that needs early folding.

> @@ -6480,7 +6480,8 @@ cp_parser_array_notation (location_t loc, cp_parser *parser, tree *init_index,
> -      *init_index = cp_parser_expression (parser);
> +      *init_index = cp_parser_expression (parser);
> +      *init_index = maybe_constant_value (*init_index);
...
> +      length_index = maybe_constant_value (length_index);
...
> +         stride = maybe_constant_value (stride);

Why fold here, rather than later when something really wants a constant? 
  If that ever actually occurs?

> +  /* 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 = maybe_constant_value (index);

Likewise.  For offsetof we should either use OFFSETOF_EXPR until late 
folding, or fold in offsetof evaluation.  I don't know why decltype 
would need anything special.  And for diagnostics we should be folding 
closer to the diagnostic.

> @@ -9876,6 +9888,7 @@ cp_parser_label_for_labeled_statement (cp_parser* parser, tree attributes)
> +       expr = maybe_constant_value (expr);

This seems redundant with the call to cxx_constant_value in case_conversion.

> @@ -12190,6 +12204,10 @@ cp_parser_static_assert(cp_parser *parser, bool member_p)
> +  /* Make sure we folded it completely before doing trying to get
> +     constant value.  */
> +  condition = fold_non_dependent_expr (condition);

This shouldn't be necessary; if the constexpr code needs to do more 
folding, that should be fixed.

> @@ -16081,6 +16099,7 @@ cp_parser_enumerator_definition (cp_parser* parser, tree type)
> +      value = maybe_constant_value (value);

This seems redundant with the call to cxx_constant_value in 
build_enumerator.

> +             width = maybe_constant_value (width);

This seems redundant with the call to cxx_constant_value in 
check_bitfield_decl.

And so on.  It seems like you added maybe_constant_value 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.

> @@ -449,7 +449,7 @@ build_aggr_init_expr (tree type, tree init)
> -    return convert (type, init);
> +    return fold (convert (type, init));

fold_convert

> @@ -3394,6 +3394,8 @@ handle_init_priority_attribute (tree* node,
> +  if (initp_expr)
> +    initp_expr = maybe_constant_value (initp_expr);

Let's use cxx_constant_value instead of this and the non-constant 
diagnostic just below.

> @@ -3371,7 +3367,7 @@ get_member_function_from_ptrfunc (tree *instance_ptrptr, tree function,
> -      e2 = fold_convert (TREE_TYPE (e3), e2);
> +      e2 = fold (convert (TREE_TYPE (e3), e2));

Why?

> @@ -3667,6 +3663,10 @@ convert_arguments (tree typelist, vec<tree, va_gc> **valu
> +      /* 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);

This should be cp_fully_fold, and lower down, after all the conversions.

> -         tree xop0 = op0, xop1 = op1, xresult_type = result_type;
> +         tree xop0 = fold (op0), xop1 = fold (op1), xresult_type = result_type;

This seems wrong.  In fact, the whole short_compare business seems like 
the sort of early folding we want to do away with.

> -  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);

What if we don't try to fold for this warning early, and instead give 
the warning later when we're folding?  I suppose that might apply to 
lots of the warnings that we're currently folding early for.

> @@ -7983,7 +7978,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);

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.

> -         gcc_assert (val1->v.val_unsigned == DWARF2_ADDR_SIZE);
> +         gcc_assert (val1->v.val_unsigned
> +                     == (unsigned HOST_WIDE_INT) DWARF2_ADDR_SIZE);

We need to fix this warning so this change is unnecessary.

> -      gcc_assert (TREE_OPERAND (t, 0) == decl);
> +      gcc_assert (TREE_OPERAND (t, 0) == decl || TREE_OPERAND (t, 1) == decl);

This change doesn't seem to have anything to do with delayed folding.

>                       || (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));

Nor this one.

> +++ b/gcc/testsuite/g++.dg/ext/offsetof1.C
> +// { dg-options "-Wno-pointer-arith" }

There isn't any user-written pointer arithmetic in this testcase, so any 
such warnings are bogus.

> +++ b/gcc/testsuite/g++.dg/warn/Wconversion-pr34389.C
> @@ -50,5 +50,5 @@ short  mask5(int x)
>
>  short  mask6(short x)
>  {
> -  return x & -1;
> +  return x & -1; // { dg-warning "conversion" }

This is also a false positive.

> +++ b/gcc/testsuite/g++.dg/warn/skip-1.C
> -// Check that we don't warn about code that will not be executed.
> +// For delayed folding we will warn about code that will not be executed too.

This is not an improvement.

> @@ -1791,6 +1791,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))

Merge error?

> @@ -1956,6 +1956,8 @@ build_complex (tree type, tree real, tree imag)
>  {
>    tree t = make_node (COMPLEX_CST);
>
> +  real = fold (real);
> +  imag = fold (imag);

I don't think we want to introduce folding into language-independent 
code like here.

> @@ -5062,6 +5063,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);

Or here.

Jason

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2015-08-28 10:15 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-12  5:41 C++ delayed folding branch review Jason Merrill
2015-06-12 16:17 ` Kai Tietz
2015-06-13  7:58   ` Jason Merrill
2015-07-27 19:01     ` Jason Merrill
2015-07-28  2:40       ` Kai Tietz
2015-07-28 20:35         ` Kai Tietz
2015-07-29 18:48           ` Jason Merrill
2015-07-29 23:03             ` Kai Tietz
2015-07-30 14:40               ` Kai Tietz
2015-07-30 18:41               ` Jason Merrill
2015-07-30 21:33                 ` Kai Tietz
2015-07-31  0:43                   ` Jason Merrill
2015-07-31  7:08                     ` Jeff Law
2015-07-31 23:00                     ` Kai Tietz
2015-08-03  3:49                       ` Jason Merrill
2015-08-03  9:42                         ` Kai Tietz
2015-08-03 15:39                           ` Jason Merrill
2015-08-24  7:20                             ` Kai Tietz
2015-08-27  2:57                               ` Jason Merrill
2015-08-27 10:54                                 ` Kai Tietz
2015-08-27 13:35                                   ` Jason Merrill
2015-08-27 13:44                                     ` Kai Tietz
2015-08-27 18:15                                       ` Kai Tietz
2015-08-28  3:03                                         ` Jason Merrill
2015-08-28  7:43                                           ` Kai Tietz
2015-08-28 11:18                                             ` Kai Tietz
2015-08-28  2:12                                       ` Jason Merrill
2015-07-31  4:00                 ` Jeff Law
2015-07-31 16:26                   ` Jason Merrill
2015-07-31 16:43                     ` Kai Tietz
2015-07-31 16:52                       ` Jakub Jelinek
2015-07-31 16:53                         ` Jason Merrill
2015-07-31 21:31                           ` Kai Tietz
  -- strict thread matches above, loose matches on Subject: below --
2015-04-24  4:23 Jason Merrill
2015-04-24 13:46 ` Kai Tietz
2015-04-24 18:25   ` Jason Merrill
2015-04-28 12:06     ` Kai Tietz
2015-04-28 13:57       ` Jason Merrill

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).