Hi,
On Sun, Nov 16, 2014 at 12:56:45AM +0100, Jan Hubicka wrote:
> Hi,
> this patch enables propagation of speculative contextes I promised to fix
> after Martin's
> merge. There were few bugs that ended up disturbing testsuite:
Wonderful, thanks a lot.
>
> 1) ipa_polymorphic_call_context::combine_with did not handle very well case
> where the incomming type is in construction and it's current type is
> known.
> This made us to drop the ball on one of devirt-*.C testcases
> 2) ipa_get_indirect_edge_target_1 did not correctly apply the offset
> adjustment
> and type_preserved prior using the known context. This caused an ICE
> while
> building Firefox
Opps, I was worried I would accidently remove something like that.
> 4) I noticed that find_more_scalar_values_for_callers_subset seems to
> contain a bug
> when searching if all incomming edges do contribute a same constant to a
> given parameter. The code seems to set the constant to NULL and then set
> it
> to non-NULL at first occurence. When it however hits two different
> constants it
> resets back to NULL and later it may get again non-NULL.
I don't there was a bug. When it was reset back to NULL, there was
the break statement that quit the loop? Your new condition (!first &&
!newval) will never trigger because in the second (or any later)
iteration, either we had hit the break statement or set newval to a
non-NULL value.
(If you however still think the code was wrong, it should be also
fixed on release branches.)
>
> Otherwise the patch just disable parts where speculation si cleared; it makes
> equal_to
> to work better and it also add meet operation that is used to compute context
> of edges
> that have multiple callers.
I admit I did not expect the ipa-cp parts to be this small.
> * ipa-polymorphic-call.c
> (ipa_polymorphic_call_context::speculation_consistent_p): Constify.
> (ipa_polymorphic_call_context::meet_speculation_with): New function.
> (ipa_polymorphic_call_context::combine_with): Handle types in
> construction
> better.
> (ipa_polymorphic_call_context::equal_to): Do not bother about useless
> speculation.
> (ipa_polymorphic_call_context::meet_with): New function.
> * cgraph.h (class ipa_polymorphic_call_context): Add
> meet_width, meet_speculation_with; constify speculation_consistent_p.
> * ipa-cp.c (ipa_context_from_jfunc): Handle speculation; combine with
> incomming
> context.
> (propagate_context_accross_jump_function): Likewise; be more cureful.
> about set_contains_variable.
> (ipa_get_indirect_edge_target_1): Fix handling of dynamic type changes.
> (find_more_scalar_values_for_callers_subset): Fix.
> (find_more_contexts_for_caller_subset): Perform meet operation.
> Index: ipa-polymorphic-call.c
> ===================================================================
> --- ipa-polymorphic-call.c (revision 217609)
> +++ ipa-polymorphic-call.c (working copy)
> @@ -1727,7 +1727,7 @@ bool
> ipa_polymorphic_call_context::speculation_consistent_p (tree spec_outer_type,
> HOST_WIDE_INT
> spec_offset,
> bool
> spec_maybe_derived_type,
> - tree otr_type)
> + tree otr_type) const
> {
> if (!flag_devirtualize_speculatively)
> return false;
> @@ -1873,6 +1873,102 @@ ipa_polymorphic_call_context::combine_sp
> return false;
> }
>
> +/* Make speculation less specific so
> + NEW_OUTER_TYPE, NEW_OFFSET, NEW_MAYBE_DERIVED_TYPE is also included.
> + If OTR_TYPE is set, assume the context is used with OTR_TYPE. */
> +
It would be nice if the return value was documented too.
> +bool
> +ipa_polymorphic_call_context::meet_speculation_with
> + (tree new_outer_type, HOST_WIDE_INT new_offset, bool
> new_maybe_derived_type,
> + tree otr_type)
> +{
> + if (!new_outer_type && speculative_outer_type)
> + {
> + clear_speculation ();
> + return true;
> + }
> +
> + /* restrict_to_inner_class may eliminate wrong speculation making our job
> + easeier. */
> + if (otr_type)
> + restrict_to_inner_class (otr_type);
> +
> + if (!speculative_outer_type
> + || !speculation_consistent_p (speculative_outer_type,
> + speculative_offset,
> + speculative_maybe_derived_type,
> + otr_type))
> + return false;
> +
> + if (!speculation_consistent_p (new_outer_type, new_offset,
> + new_maybe_derived_type, otr_type))
> + {
> + clear_speculation ();
> + return true;
> + }
> +
> + else if (types_must_be_same_for_odr (speculative_outer_type,
> + new_outer_type))
> + {
> + if (speculative_offset != new_offset)
> + {
> + clear_speculation ();
> + return true;
> + }
> + else
> + {
> + if (!speculative_maybe_derived_type && new_maybe_derived_type)
> + {
> + speculative_maybe_derived_type = true;
> + return true;
> + }
> + else
> + return false;
> + }
> + }
> + /* See if one type contains the other as a field (not base). */
> + else if (contains_type_p (new_outer_type, new_offset - speculative_offset,
> + speculative_outer_type, false, false))
> + return false;
> + else if (contains_type_p (speculative_outer_type,
> + speculative_offset - new_offset,
> + new_outer_type, false, false))
> + {
> + speculative_outer_type = new_outer_type;
> + speculative_offset = new_offset;
> + speculative_maybe_derived_type = new_maybe_derived_type;
> + return true;
> + }
> + /* See if OUTER_TYPE is base of CTX.OUTER_TYPE. */
> + else if (contains_type_p (new_outer_type,
> + new_offset - speculative_offset,
> + speculative_outer_type, false, true))
> + {
> + if (!speculative_maybe_derived_type)
> + {
> + speculative_maybe_derived_type = true;
> + return true;
> + }
> + return false;
> + }
> + /* See if CTX.OUTER_TYPE is base of OUTER_TYPE. */
> + else if (contains_type_p (speculative_outer_type,
> + speculative_offset - new_offset, new_outer_type,
> false, true))
> + {
> + speculative_outer_type = new_outer_type;
> + speculative_offset = new_offset;
> + speculative_maybe_derived_type = true;
> + return true;
> + }
> + else
> + {
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + fprintf (dump_file, "Giving up on speculative meet\n");
> + clear_speculation ();
> + return true;
> + }
> +}
> +
> /* Assume that both THIS and a given context is valid and strenghten THIS
> if possible. Return true if any strenghtening was made.
> If actual type the context is being used in is known, OTR_TYPE should be
> @@ -2160,19 +2266,220 @@ ipa_polymorphic_call_context::equal_to
> else if (x.outer_type)
> return false;
>
> - if (speculative_outer_type)
> +
> + if (speculative_outer_type
> + && speculation_consistent_p (speculative_outer_type,
> speculative_offset,
> + speculative_maybe_derived_type, NULL_TREE))
> {
> - if (!x.speculative_outer_type
> - || !types_odr_comparable (speculative_outer_type,
> - x.speculative_outer_type)
> + if (!x.speculative_outer_type)
> + return false;
> +
> + if (!types_odr_comparable (speculative_outer_type,
> + x.speculative_outer_type)
> || !types_same_for_odr (speculative_outer_type,
> - x.speculative_outer_type)
> + x.speculative_outer_type)
> || speculative_offset != x.speculative_offset
> || speculative_maybe_derived_type != x.speculative_maybe_derived_type)
> return false;
> }
> - else if (x.speculative_outer_type)
> + else if (x.speculative_outer_type
> + && x.speculation_consistent_p (x.speculative_outer_type,
> + x.speculative_offset,
> + x.speculative_maybe_derived_type,
> + NULL))
> return false;
Do we often leak speculative-inconsistent contexts outside of this
file?
>
> return true;
> }
> +
> +/* Modify context to be strictly less restrictive than CTX. */
Documenting the return value would be nice here too.
Thanks,
Martin
