Re: C++ delayed folding branch review

[Kai Tietz <ktietz70 at googlemail dot com>]

2015-04-24 20:25 GMT+02:00 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.

Ok.  Just a question about where to hook up the hash-variable.  What
would be a good place to create (well this we could do lazy too) it,
and of more importance where to destroy the hash? We could destroy it
after genericize-pass?

>>>> @@ -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, beside an overflow was seen.  The interesting point is here to
find the proper place to do for constructor-elements proper folding,
or at least constant-value folding.  I made here already some tries to
find a proper place for this, but ran by this into troubles that not
in all cases maybe_constant_value was callable (endless recursion).
Additionally it happens in some cases for constructor-elements that
nop-casts are added on assign.  At least I ran into that.
In general there is the question if we shouldn't do for constructors
call a ...fully_fold?

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

I tested to use here just a fold (), and I ddn't noticed any fallout
by this.  So I committed it to branch.

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

maybe_constant_value folds constants, too.  That was actually the
reason to use it.  Nevertheless I admit that we could call instead a
..fully_fold here too.
One point I see here about creating a function using
maybe_constant_value + cp_fold is that maybe_constant_value is
something we can call safely in all cases.

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

Agreed, that bugs like PR 53792 could be solved that way.
Nevertheless they aren't directly linked to the delayed-folding
problem, are they?  We could deal with those cases also in
genericize-pass lately, as we want to catch here just a missed
optimization, aren't we?

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

Fine, any preferences about place for this function?  I suggest the
name 'fold_cst' for it.

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

We just call it in none-dependent tree, and even here just for special
cases.  We need to make sure to simplifiy to constant.  Well, we could
use here instead fold_cst instead (which calls internally
...fully_fold, but just returns altered tree if result is constant).

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

Hmm, I doubt that a simple fold would be enough here.  As itype is
calculated as minux of two converts (and cp_converts aren't
automatically folded).

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

I am testing

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

Well, issue is that cxx_constant_value is just called for
!processing_template_decl case. Additionally we try to
perform_implicit_conversion_flags on it, which can add new
cast-operation, which gets resulved by call for cxx_constant_value.
So it makes a difference AFAIC, but I might misread here

>
> How does delayed folding result in a different OMP_FOR_KIND?

Well,  I guess it is related to optimizations/normalization done early
on parsing-state, which isn't triggered anymore on later folds.  As
OMP-stuff isn't that good reading, it is hard to tell why
normalization doesn't happen here anymore.  I will try to look into
that, but not sure if it is worth the effort at the very end ...

>
> Jason
>

Kai