C++ delayed folding branch review

[Jason Merrill]

On 07/29/2015 06:56 PM, Kai Tietz wrote:
>>>>>>>>> @@ -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);
>
> Checked, and removing those STRIP_NOPS cause regressions about
> vector-casts.  At least the STRIP_NOPS in
> reduced_constant_expression_p seems to be required.  See as example
> g++.dg/ext/vector20.C as testcase.
> It sees that '(vec)(const __vector(2) long int){3l, 4l}' is not a
> constant expression.

But when was that NOP_EXPR added?  It should have been folded away 
before we get here.

>>>>>>>>>         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?
>
> The issue is that by delayed-folding we don't fold sizeof-expressions
> until we do the folding after genericize-pass.  So those expressions
> remain, and we can run in template on sizeof-operators on incomplete
> types, if we invoke here variants of the constexpr-code.  So this
> pattern simply verifies that the sizeof-operand can be determined.  We
> could simply avoid resolving sizeof-operators in template-decl at all.
> But my idea here was to try to resolve them, if the type of the
> operand is already complete (and has an constant size).

But this condition will never be true, as TREE_TYPE (t) is always 
size_t.  So this code isn't actually addressing the situation you describe.

>>>>>>> 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.
>
> Well, but we could come here by other routine then
> maybe_constant_value. For example cxx_constnat_value doesn't do checks
> here.

Calling cxx_constant_value on a dependent expression will tend to ICE, 
so we don't need to worry about that.

>>>>>>>>> @@ -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.
>
> If we need to do some reduction to constant value here, as expr might
> be actually a constant, which isn't folded here.  Eg something like:
> struct {
>    char abc[sizeof (int) * 8];
> };
> Due delayed folding array index isn't necessarily reduced here.  So we
> need to perform at least constant value folding for diagnostics, as we
> do right now.

Yes, we need to do some folding, that's why we call maybe_constant_value!

>>>>>>>>> @@ -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
>>
>> Looks like this hasn't changed.
>
> Yes, for grokbitfield current version uses fold_simple for witdth.  So
> just expressions based on constants getting reduced to short form.  In
> grokbitfield I don't see invocation of cxx_constant_value.  So how can
> we be sure that width is reduced to integer-cst?

We call cxx_constant_value on bit-field widths in check_bitfield_decl.

>>>>>>>>> @@ -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.
>>
>> Looks like the fold is still there.
>
> Yes, but a fold_simple one just working on constant values.  It
> doesn't fold expressions like 'a == a' to a constant.  I extended
> comment in current version on branch.  Additionally it invokes now the
> fold_simple always.

> We want to reduce index, if possible, for
> diagnostics in code in c-family/c-common.c

Why not closer to the diagnostics?

> for array-bounds,

We already fold array bounds.

> for types (they need to be fully folded)

WHY?  How many times do I need to ask you for SOME reason?  You keep 
just saying it's necessary without any evidence.

> and to be sure we simplify basic operations on constant-values.

Why here, rather than closer to where we care about such simplification?

Again:

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


>>>> 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).
>>
>> OK, but this seems like an typical case of needing to fold for diagnostics;
>> usually in those cases you use the folded value for the diagnostics and then
>> keep using the unfolded expression elsewhere.
>
> Right.

So are you going to make that change here?

>>>> 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?
>>
>> Why does the fold need to be in the parser?
>
> Well, if we hit it during our tree-walk in cp_fold_r, then we don't
> need to fold it here.  I will check, if this is really necessary.

?

>>>>>>>>> @@ -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.
>>
>> ?
>
> This hunk is necessary as we don't use canonical-form produced by
> shorten_compare anymore.  Therefore special operand can occur on
> right-hand side too.

That seems like a problem, if the middle end is expecting the canonical 
form.  What is your plan for dealing with shorten_compare issues, again?

>>>>>>>>> @@ -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?
>>
>> Yes.
>
> Applied modification to it.

So can we remove the fold in build_complex now?

>>>>>> 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.
>>
>> convert is called from the ME, which sometimes expects folding.
>>
>>>> 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 was thinking of convert.c and fold-const.c to be part of the ME, since
>> they are language-independent.  But I guess other people think of the ME
>> starting with gimple.
>>
>> And it looks like the only language-independent uses of convert are in
>> c-family; I guess many of them should change to fold_convert.
>
> Hmm, in context of this work? Or is this more a general point about future work?

In the context of this work, if they are introducing problematic NOPs.

>>>>>>>>> @@ -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.
>
> I didn't come to remove this line for testing.  As we fold now for
> initializers more early, and cp_fold supports constructors, it could
> be that we don't need this anymore.  It is on my pile.

> That fold is still required.  By removing it, I saw boostrap issue due
> 'invalid initializer'.

That indicates a folding problem earlier on, that will cause some 
initialization that should be performed at compile time to happen at run 
time instead.

Please investigate the bootstrap issue further.

>>>>>>> @@ -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);
>>>>>>
>>>>>> And here, convert_nontype_argument already uses
>>>>>> maybe_constant_value/cxx_constant_value for folding constants.
>
> Yes, this invocation looks useless too.  I think I introduced it for
> the STRING_CST check below, but AFAICS we should assume it as
> unnecessary.  I will change it and do regression-testing.

Is this still in process?

> By recent changes we seem to hit for c++ some additional regression.
> They are related to negate-shifts for c++11.  We are hitting now the
> check within cxx_constant_value.  The cxx_eval_check_shift_p sees now
> that left-hand operand is negative and produces two new errors for
> following tests: c-c++-common Wshift-negative-value-*.c cases.

Which lines are affected?  Are the new messages correct or not?

> So we will need adjust those cases, or invoke within this
> eval-function instead maybe_constant_value to avoid that ?

This eval function is part of constexpr evaluation, so it wouldn't make 
sense to call maybe_constant_value here.  And the arguments to this 
function have just gone through cxx_eval_constant_expression.

Jason