Re: C++ delayed folding branch review

[Jason Merrill <jason@redhat.com>]

I've trimmed this to the previously mentioned issues that still need to 
be addressed; I'll do another full review after these are dealt with.

On 06/13/2015 12:15 AM, Jason Merrill wrote:
> On 06/12/2015 12:11 PM, Kai Tietz wrote:
>>>> @@ -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 ...
>
> We shouldn't within the constexpr code.  NOPs for expressions that are
> non-constant due to overflow are added in
> cxx_eval_outermost_constant_expr, so we shouldn't see them in the middle
> of constexpr evaluation.
>
>>>> @@ -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.
>
> If this code path is hit, that means something has broken my design, and
> I don't want to just paper over that.  Please revert this change.
>
>>>>       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).
>
> Yes, but we shouldn't have gotten this far with a dependent sizeof;
> maybe_constant_value just returns if
> instantiation_dependent_expression_p is true.
>
>>>> @@ -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.
>
> How does it need to be handled?  A NOP_EXPR wrapped around an overflow
> is there to indicated that the expression is non-constant, and it can't
> be simplified any farther.
>
> Please give an example of what was going wrong.
>
>>>> @@ -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.
>
> Hmm, I wonder why you would see issues with global initializers that
> aren't seen on trunk?  In any case, if the issue is with global
> initializers, they should be handled sooner, not here.
>
>>>> @@ -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 ...

Now we've been talking about calling it "fold_simple".

>>>> @@ -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.
>>
>> Well, 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.
>
> Again, null_node is special.  It indicates that the user typed "__null".
> That's what we're checking for here.  Folding is both unnecessary and
> undesirable.
>
>>>> @@ -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 ...
>
> No.  Again, in C++11 only "0" or "0L" is a null pointer constant.   A
> more complex expression that folds to 0 is NOT a null pointer constant.
> Folding is actively harmful here.
>
> And again, in C++98 mode null_ptr_cst_p already folds, so doing it here
> is redundant.
>
> Was I unclear?
>
>>>> @@ -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 ...
>
> If someone tries to create an array with a size that involves arithmetic
> overflow, that's undefined behavior and we should probably give an error
> rather than fold it away.
>
>>>> @@ -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 ...
>
> The cxx_constant_value call below will deal with them.

Likewise for grokbitfield.

>>>> @@ -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.
>
> They shouldn't be ignored.  C++ requires that the value be constant, and
> overflow makes it non-constant.
>
>> I will recheck this what example we have for this when bootstrap is
>> working again.
>>
>>>> @@ -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.
>
> I don't think this is as soon as possible; we can fold the NEGATE_EXPR
> immediately when we build it, at the end of cp_build_unary_op.
>
> I still wonder why any folding is necessary for decltype.  When I ask
> why, I want to know *why*, not just have you tell me again that it's
> needed.  I don't think it is.
>
> For offsetof, I wonder if it makes sense to extend fold_offsetof_1 to
> handle whatever additional folding is needed here.  If not, then fold in
> finish_offsetof, before calling fold_offsetof.

I see that this is now an unconditional fold_simple, but I still don't 
understand why it needs to be folded here, in the parser.

 >....
>> 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.
>
> I want to delay it to:
>
> 1) the places where we actually care about constant values, all of which
> already call maybe_constant_value or cxx_constant_value, so they
> shouldn't need much change; and
> 2) the places where we want a simplified expression for warnings, where
> we should call fold_simple.

> Folding in the parser is wrong, most of all because template
> substitution doesn't go through the parser.

There are still several folds in cp_parser_omp_* that should move later.

>  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;
> +
> +  expr_ovl = fold_simple (expr_ovl);
> +  STRIP_NOPS (expr_ovl);

Why both fold_simple and STRIP_NOPS?

>>>> @@ -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.
>
> But why here?  Can't we fold away useless casts earlier (in convert) or
> later (when we care about having a simplified expression)?
>
>>>> @@ -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.

I would think this should be maybe_constant_value then.

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

Introducing *_non_dependent_expr is definitely wrong here.

>>>> @@ -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.
>
> Why not?  Doesn't it fold all expressions?

?

>> 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.
>
> That seems like a problem.

> @@ -867,7 +867,7 @@ expand_subword_shift (machine_mode op1_mode, optab binoptab,
>          are truncated to the mode size.  */
>        carries = expand_binop (word_mode, reverse_unsigned_shift,
>                               outof_input, const1_rtx, 0, unsignedp, methods);
> -      if (shift_mask == BITS_PER_WORD - 1)
> +      if (shift_mask == (unsigned HOST_WIDE_INT) (BITS_PER_WORD - 1))

These should still be unnecessary.

>>>> @@ -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.
>
> Is this because of the
>>       value = build_complex (NULL_TREE, convert (const_type,
>>                                                  integer_zero_node),
>> value);
> in interpret_float?  I think "convert" definitely needs to do some
> folding, since it's called from middle-end code that expects that.

I remember talking about "convert" doing some folding (and cp_convert 
not) in our 1:1 last week.

>>>> @@ -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.
>
> Yep, they need to be folded before we get here.
>
> It looks like your latest checkin added more redundant folding:
>
>> @@ -3311,6 +3311,9 @@ finish_case_label (location_t loc, tree
>> low_value, tree hi
>> gh_value)
>>    low_value = case_conversion (type, low_value);
>>    high_value = case_conversion (type, high_value);
>>
>> +  low_value = cp_fully_fold (low_value);
>> +  high_value = cp_fully_fold (high_value);
>
> Again, case_conversion should have already folded constants.
>
>> @@ -5776,6 +5776,8 @@ convert_nontype_argument (tree type, tree expr,
>> tsubst_flags_t complain)
>>  {
>>    tree expr_type;
>>
>> +  expr = cp_try_fold_to_constant (expr);
>> +
>>    /* Detect immediately string literals as invalid non-type argument.
>>       This special-case is not needed for correctness (we would easily
>>       catch this later), but only to provide better diagnostic for this
>> @@ -5852,6 +5854,7 @@ convert_nontype_argument (tree type, tree expr,
>> tsubst_flags_t complain)
>>        else if (TYPE_PTR_OR_PTRMEM_P (type))
>>         {
>>           tree folded = maybe_constant_value (expr);
>> +         folded = cp_try_fold_to_constant (expr);
>
> And here, convert_nontype_argument already uses
> maybe_constant_value/cxx_constant_value for folding constants.

Jason