Re: C++ delayed folding branch review

[Jason Merrill <jason@redhat.com>]

On 04/24/2015 09:46 AM, Kai Tietz wrote:
> Sure, we can use here instead *_fully_fold, but for what costs?  In
> general we need to deal here a simple one-level fold for simplifying
> constant-values, and/or removing useless type-conversions.

Well, here you're doing a two-level fold.  And in general fold relies on 
subexpressions being appropriately folded.  So for warning folding, I 
think the caching approach we were talking about in the call today is 
the way to go.

>>> @@ -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.
>
> s.o. we need to make sure constant-values get rid of useless
> types-conversions/negates/etc ...

Certainly a constant value from maybe_constant_value should not have a 
useless type conversion wrapped around it, as that makes it appear 
non-constant.

> Well, fold_convert isn't necessarily the same as fold (convert ())
> within C++, due convert handles special cases fold_convert doesn't.

Ah, true.

>>> @@ -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.
>
> Ok.  I remove this fold-case due simply removing
> fold_if_not_in_template function.  So well, we could re-add a call for
> fold, if not in template.

Let's try not checking for being in a template, see if it breaks.

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

Done.

>>> +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.
>
> Well, the functionality of cp_fold and maybe_constant_value (well,
> actually how constexpr.c works) are different in cases of
> non-constant results.

I think that's exactly what I was saying above: "we can't just use
maybe_constant_value because that only folds C++ constant-expressions, 
and we want to fold more things than that."

My point is that cp_fold should be a superset of maybe_constant_value, 
to fix bugs like 53792.  And the easiest way to get that would seem to 
be by calling 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.

> See, that we want to take care that constant-value is found here.
> Otherwise we don't want anything folded.   Well, we could introduce
> for this a special routine to abstract intention here.

OK, that makes sense.  Say, a function called like "fold" that only 
folds conversions (and NEGATE_EXPR) of constants.  It might make sense 
to do that and otherwise continue to delay folding of conversions.  In 
that context I guess this change makes sense.

>>> @@ -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.
>
> Well, see above.  We might have constant-value not simplified.  So we
> need a way to make sure we simplify in such case, but if it is
> none-constant, we don't want an modified expression.  So
> maybe_constant_value does this ...

Yes, but we already called maybe_constant_value.  Calling it again 
shouldn't make any difference.

>>> -      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?
>
> We could use fully_fold, but we would also modify none-constant
> expressions by this.  Do we actually want that here?

For the first one, I actually think a plain fold is enough, or perhaps 
change the cp_build_binary_op to size_binop.

For the second, can we drop that whole block (the one starting with "if 
(TREE_CODE (itype) != SAVE_EXPR)") and rely on late folding to handle 
SIZEOF_EXPR?

>>> @@ -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.
>
> See the other places

As above, calling the constexpr code twice shouldn't make a difference.

>>> -         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.
>
> Well, those following changes got already acked by Jeff for 4.9.

Because we want the compiler to build with a wide range of other 
compilers.  It's still a warning regression that needs to be fixed.

>>> -      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.
>
> It gets raised by delayed folding, so it is required to run.

We're in gimplification, so we should have been through cp_fold, so this 
should have been folded appropriately by now.

>>>                        || (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.
>
> Same thing ...

How does delayed folding result in a different OMP_FOR_KIND?

Jason