This is the mail archive of the
mailing list for the GCC project.
Re: C++ delayed folding branch review
- From: Kai Tietz <ktietz70 at googlemail dot com>
- To: Jason Merrill <jason at redhat dot com>
- Cc: gcc-patches List <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 28 Apr 2015 14:06:21 +0200
- Subject: Re: C++ delayed folding branch review
- Authentication-results: sourceware.org; auth=none
- References: <5539C519 dot 8070004 at redhat dot com> <CAEwic4YZBXncYCrfCcT42n_+8qbf3n_ZKiDNohvGzvp2A4YfZw at mail dot gmail dot com> <553A8A8E dot 8070403 at redhat dot com>
2015-04-24 20:25 GMT+02:00 Jason Merrill <firstname.lastname@example.org>:
> 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
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
>>>> @@ -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
>>> 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
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
>>> 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
>>> folding if we aren't doing constexpr evaluation. But we can't just use
>>> maybe_constant_value because that only folds C++ constant-expressions,
>>> 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)
>>>> - 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
>> 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
> 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
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 ...