C++ delayed folding branch review
Jason Merrill
jason@redhat.com
Fri Apr 24 18:25:00 GMT 2015
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
More information about the Gcc-patches
mailing list