Re: C++ delayed folding branch review

[Kai Tietz <ktietz70@googlemail.com>]

2015-07-28 1:14 GMT+02:00 Kai Tietz <ktietz70@googlemail.com>:
> 2015-07-27 18:51 GMT+02:00 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.
>
> Thanks for doing this summary of missing parts of prior review.
>
>> 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".
>
> Yes, rename happened. I will send a patch for that.  By reading
> cp_fold_convert, I agree that folding can be assumed ....

Changed that on branch.  Users of cp_fold_convert are looking to me
just like artificial-code-generations, and IMO we can keep there usage
of cp_fold_convert.

>>
>>>>>> @@ -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.
>
> Hmm, AFAIR we don't call cxx_constant_value in all code-paths.  But I
> will look into it, and come back to you on it.

I am still on it ...  first did the other points

>>>>>> @@ -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.
>
> The point to fold the 'value' here is for cases
> 'processing_template_decl' isn't false. We could move it to the
> else-case of the 'if (! processing_template_decl)' line for being more
> explicit?

Well, on looking here in more detail, we might don't that that initial
folding here.  As for processing_template_decl fold_simple (and
cp_fully_fold) doesn't do much.

>>>....
>>>>
>>>> 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.
>
> In 'cp_parser_omp_var_list_no_open' we need to fold 'length' can
> 'low_bound' as those values getting checked some lines below (see
> lines 27936, 27944). We could call here fold_simple instead?
> If we would delay it to later, we would need to move diagnostics here too,

?

> In 'cp_parser_omp_clause_aligned','cp_parser_omp_clause_safelen', and
> in 'cp_parser_omp_clause_simdlen' I tried to fold early to prevent
> early cases assuming that OMP-operands are already folded.
> I tested this, and it doesn't seems to be necessary anymore due we
> perform full converatge here in the cp_fold_r walker now.
> I removed here the fold-invocation of them.
>
> In 'cp_parser_cilk_grainsize' we fold 2nd argument of
> 'cp_paser_cild_for' by 'fold_simple'.  Not sure if it is worth to move
> operand-folding into cp_parser_cilk_for itself, as we have here just
> two users of 'cp_parser_cilk_for'.
> One time we pass 'integer_zero_node' as this argument, and the other
> time a binary-expression, which might be constant value.
> But sure we can move it into 'cp_parser_cilk_grainsize'.if you prefer?

?

>>>  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?
>
> If we have an overflow-value encapsulated into an nop_expr, we want to
> see that overflow-expression itself.
> fold_simple preserves the nop_expr conversion, so we want to remove
> it, if present.
> Not sure, if this really still can happen, I will do some testing on it.

No, it doesn't happen anymore.  I removed STRIP_NOP.

>>>>>> @@ -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.
>
> Why?  At the end we resolve normal-builtin via 'fold_call_expr'.  Of
> course we can invoke here maybe_constant_value, but it would end up in
> the same folding of a builtin-expression. So calling here directly
> 'fold' just short-cuts this.

?

>
>>>>>> @@ -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.
>
> I don't remember anymore, why I used here *_non_dependent_expr.  I
> will see if we can use here instead cp_fully_fold.  If we can get rid
> of the STRIP_NOPs, I am not sure, as we are interested in
> overflow-bit, and don't want to see it encapsulated into nop_expr ...

I replaced those *_non_dependent_expr calls by fold_simple calls.  The
STRIP_NOPS isn't necessary then too.

>>
>>>>>> @@ -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?
>>
>>
>> ?
>
> It fold them lately.  I will recheck this code-change.  It might be no
> longer required due recent changes to omp-folding.  It could be that
> original pattern didn't applied here anymore, and therefore statement
> didn't been transformed into its canonical form.  Bit I assume this
> could be resolved.
>
>>>> 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.
>
> See above.  I will come up on this tomorrow.

I think this is resolved already.  It was related to folding of OMP
conditions.  I will do bootstrap with this humk removed.

>>> @@ -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.
>
> Yes, they are.  We handle now the avoiding of dead-code for constants
> (cond-expression, truthif* expressions, and useless convert-warnings).
> So this change is something only interesting for different compiler,
> but not related to delayed-folding anymore.
> I will check, and remove it tomorrow.
>
>>>>>> @@ -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);
>
> Might be.  This should be indeed a 'fold_convert', isn't it?
>
>>> 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.
>>
>
> Can't remember that.  I know that we were talking about the difference
> of convert and fold_convert.  convert can be used on C++ specifics,
> but fold_convert is something shared with ME.  So first 'fold_convert'
> isn't the same as 'fold (convert ())'.
> I don't find places we invoke convert () in ME.  We have some calls in
> convert.c (see convert_to_integer, convert_to_integer_nofold, and
> convert_to_real), which all used in AST only AFAICS.
> I remember that we were talking about adding a standard-folding to
> convert for operations on constant-values (as we do for
> convert_to_integer).  Do you mean this?
>
>>>>>> @@ -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
>>
>

Kai