Re: [PATCH 3/9] operand_equal_p: add support for OBJ_TYPE_REF.
On Fri, Sep 13, 2019 at 2:30 PM Martin Liška wrote: > > On 8/20/19 8:39 AM, Richard Biener wrote: > > Anyhow, the original patch is OK if you compare > > OBJ_TYPE_REF_TOKEN/OBJECT irrespective of virtual_method_call_p > > and order the types_same_for_odr last since that's most expensive. > > Hi. > > It's done in the attached patch that survives bootstrap and regression > tests. OK. Richard. > Martin
Re: [PATCH 3/9] operand_equal_p: add support for OBJ_TYPE_REF.
On 8/20/19 8:39 AM, Richard Biener wrote:
Anyhow, the original patch is OK if you compare
OBJ_TYPE_REF_TOKEN/OBJECT irrespective of virtual_method_call_p
and order the types_same_for_odr last since that's most expensive.
Hi.
It's done in the attached patch that survives bootstrap and regression
tests.
Martin
>From 645e2df84ccd1f9a4b41f0a73a5398ff81696cdc Mon Sep 17 00:00:00 2001
From: Martin Liska
Date: Thu, 15 Aug 2019 10:34:41 +0200
Subject: [PATCH 3/9] operand_equal_p: add support for OBJ_TYPE_REF.
gcc/ChangeLog:
2019-07-24 Martin Liska
* fold-const.c (operand_equal_p): Support OBJ_TYPE_REF.
* tree.c (add_expr): Hash parts of OBJ_TYPE_REF.
---
gcc/fold-const.c | 18 ++
gcc/tree.c | 6 ++
2 files changed, 24 insertions(+)
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index f57fffb9655..12f5a06a524 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -3325,6 +3325,24 @@ operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
flags &= ~OEP_ADDRESS_OF;
return OP_SAME (1) && OP_SAME (2);
+ /* Virtual table call. */
+ case OBJ_TYPE_REF:
+ {
+ if (!operand_equal_p (OBJ_TYPE_REF_EXPR (arg0),
+ OBJ_TYPE_REF_EXPR (arg1), flags))
+ return false;
+ if (tree_to_uhwi (OBJ_TYPE_REF_TOKEN (arg0))
+ != tree_to_uhwi (OBJ_TYPE_REF_TOKEN (arg1)))
+ return false;
+ if (!operand_equal_p (OBJ_TYPE_REF_OBJECT (arg0),
+ OBJ_TYPE_REF_OBJECT (arg1), flags))
+ return false;
+ if (!types_same_for_odr (obj_type_ref_class (arg0),
+ obj_type_ref_class (arg1)))
+ return false;
+ return true;
+ }
+
default:
return false;
}
diff --git a/gcc/tree.c b/gcc/tree.c
index b5e5876bbb3..20eb1682435 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -8028,6 +8028,12 @@ add_expr (const_tree t, inchash::hash , unsigned int flags)
inchash::add_expr (TARGET_EXPR_SLOT (t), hstate, flags);
return;
+ /* Virtual table call. */
+ case OBJ_TYPE_REF:
+ inchash::add_expr (OBJ_TYPE_REF_EXPR (t), hstate, flags);
+ inchash::add_expr (OBJ_TYPE_REF_TOKEN (t), hstate, flags);
+ inchash::add_expr (OBJ_TYPE_REF_OBJECT (t), hstate, flags);
+ return;
default:
break;
}
--
2.23.0
Re: [PATCH 3/9] operand_equal_p: add support for OBJ_TYPE_REF.
> I see. I guess dropping them if !virtual_method_call_p (at what point > do we know?) would be a good thing. As well as encoding > "types_same_for_odr" and obj_type_ref_class in a more direct manner. > I guess in reality OBJ_TYPE_REF should be all info on the > gimple_call rather than in a GENERIC tree in the call fn slot or > a separate stmt ... > > Anyhow, the original patch is OK if you compare > OBJ_TYPE_REF_TOKEN/OBJECT irrespective of virtual_method_call_p > and order the types_same_for_odr last since that's most expensive. > I also wonder if virtual_method_call_p needs to return a virtual_mehtod_call_p is constant on the expression thorough compilation. I.e. if OBJ_TYPE_REF is born in C++ FE it will return true if it is Obj-C FE it will return false and we will completely ignore that OBJ_TYPE_REF in the middle-end (and thus we probably want to drop it - I was just never brave enough to dig into obj-C gimplification to figure out where it should be done and I also do not know how OBJ-C uses OBJ_TYPE_REFs internally if at all) Honza > "maybe" and we have to say not equal in that case rather than just > not comparing obj_type_ref_class ... (operand_equal_p might be called > from FEs during parsing) > > Richard. > > Thanks, > Richard. > > > Honza
Re: [PATCH 3/9] operand_equal_p: add support for OBJ_TYPE_REF.
On Mon, Aug 19, 2019 at 4:34 PM Jan Hubicka wrote: > > > On Fri, Aug 16, 2019 at 2:07 PM Jan Hubicka wrote: > > > > > > > > > > > When we compare OBJ_TYPE_REF_TOKEN and OBJ_TYPE_REF_OBJECT > > > > and they are equal, are there cases where types_same_for_odr returns > > > > false? > > > > > > OBJ_TYPE_REF_OBJECT is pointer to the instance, OBJ_TYPE_REF_TOKEN is > > > index in the vtable or the type given by obj_ref_type_class. I guess > > > one can do something like > > > > > > void *ptr; > > > ... > > > if (cond) > > >((class_a *)ptr)->firstvirtualmethod_of_class_a (); > > > else > > >((class_b *)ptr)->firstvirtualmethod_of_class_b (); > > > > > > Here OBJECT will be always *ptr, TOKEN will be 0, but the actual virtual > > > method is different. So merging this may lead to invalid > > > devirtualization at least when the classes are anonymous namespace and > > > we work out late in compilation that they are not derived. > > > > But we also compare OBJ_TYPE_REF_EXPR and later we expand to a call > > with exactly that address... > > I think this is same as with memory references. Just because the > addresses compare equal and read same type we still can not merge w/o > verifying that the alias oracle will not give different answers > (so we need to compare cliques and access paths). operand_equal_p does > checking in this case though it is bit random on way it understands > access paths. > > To get more agressive unification we can drop the optional metadata > (i.e. remove OBJ_TYPE_REF or drop to alias set zero) while merging but I > think this will need more care and decisions what to do for -Os only and > what to do for -O2/fast. For this reason I would first handle this > conservatively (i.e. require match of metadata as well) and then improve > from that. I see. I guess dropping them if !virtual_method_call_p (at what point do we know?) would be a good thing. As well as encoding "types_same_for_odr" and obj_type_ref_class in a more direct manner. I guess in reality OBJ_TYPE_REF should be all info on the gimple_call rather than in a GENERIC tree in the call fn slot or a separate stmt ... Anyhow, the original patch is OK if you compare OBJ_TYPE_REF_TOKEN/OBJECT irrespective of virtual_method_call_p and order the types_same_for_odr last since that's most expensive. I also wonder if virtual_method_call_p needs to return a "maybe" and we have to say not equal in that case rather than just not comparing obj_type_ref_class ... (operand_equal_p might be called from FEs during parsing) Richard. Thanks, Richard. > Honza
Re: [PATCH 3/9] operand_equal_p: add support for OBJ_TYPE_REF.
> On Fri, Aug 16, 2019 at 2:07 PM Jan Hubicka wrote: > > > > > > > > When we compare OBJ_TYPE_REF_TOKEN and OBJ_TYPE_REF_OBJECT > > > and they are equal, are there cases where types_same_for_odr returns > > > false? > > > > OBJ_TYPE_REF_OBJECT is pointer to the instance, OBJ_TYPE_REF_TOKEN is > > index in the vtable or the type given by obj_ref_type_class. I guess > > one can do something like > > > > void *ptr; > > ... > > if (cond) > >((class_a *)ptr)->firstvirtualmethod_of_class_a (); > > else > >((class_b *)ptr)->firstvirtualmethod_of_class_b (); > > > > Here OBJECT will be always *ptr, TOKEN will be 0, but the actual virtual > > method is different. So merging this may lead to invalid > > devirtualization at least when the classes are anonymous namespace and > > we work out late in compilation that they are not derived. > > But we also compare OBJ_TYPE_REF_EXPR and later we expand to a call > with exactly that address... I think this is same as with memory references. Just because the addresses compare equal and read same type we still can not merge w/o verifying that the alias oracle will not give different answers (so we need to compare cliques and access paths). operand_equal_p does checking in this case though it is bit random on way it understands access paths. To get more agressive unification we can drop the optional metadata (i.e. remove OBJ_TYPE_REF or drop to alias set zero) while merging but I think this will need more care and decisions what to do for -Os only and what to do for -O2/fast. For this reason I would first handle this conservatively (i.e. require match of metadata as well) and then improve from that. Honza
Re: [PATCH 3/9] operand_equal_p: add support for OBJ_TYPE_REF.
On Fri, Aug 16, 2019 at 2:07 PM Jan Hubicka wrote: > > > > > When we compare OBJ_TYPE_REF_TOKEN and OBJ_TYPE_REF_OBJECT > > and they are equal, are there cases where types_same_for_odr returns false? > > OBJ_TYPE_REF_OBJECT is pointer to the instance, OBJ_TYPE_REF_TOKEN is > index in the vtable or the type given by obj_ref_type_class. I guess > one can do something like > > void *ptr; > ... > if (cond) >((class_a *)ptr)->firstvirtualmethod_of_class_a (); > else >((class_b *)ptr)->firstvirtualmethod_of_class_b (); > > Here OBJECT will be always *ptr, TOKEN will be 0, but the actual virtual > method is different. So merging this may lead to invalid > devirtualization at least when the classes are anonymous namespace and > we work out late in compilation that they are not derived. But we also compare OBJ_TYPE_REF_EXPR and later we expand to a call with exactly that address... > > > > I guess we went over this some time ago when talking about value-numbering > > of them. I realize the devirt machinery use the class type to get at the > > virtual table but since we later expand simply OBJ_TPE_REF_EXPR _that_ > > already has the result of the load from the vitual table. > > > > So - do we need to compare obj_type_ref_class at all? The worst thing > > that could happen is that devirt no longer can devirtualize after the > > merging > > but it should never result in "wrong" devirtualization? So, do we really > > want to inhibit ICF of two equal functions that could be eventually > > devirtualized both but into different direct calls? Can't this sort of > > thing > > I think it is like with the TBAA info. I would suggest first implement > tests that preserve correctness of both TBAA and devirt annotations. > Incrementally we can teach ICF to be agressive with -Os and merge anyway > while dropping the metadata (removing OBJ_TYPE_REF or adjusting alias > sets > > happen anyway when you'd factor in IPA-CP and two identical functions > > with indirect calls to a function argument but called with different > > constant > > args? And should IPA-CP then not consider cloning after ICF merging > > (not sure how ordering works here). > > I am not sure how those are realated. For sure IPA-CP can clone when it > works out that different types are passed to the function and > devirtualize to different calls. But this is bit different - it is about > the type annotation we > > Honza > > > > Richard. > > > > > Honza > > > > > > > > + } > > > > > > > > > > > > > 2019-07-24 Martin Liska > > > > > > > > > > * fold-const.c (operand_equal_p): Support OBJ_TYPE_REF. > > > > > * tree.c (add_expr): Hash parts of OBJ_TYPE_REF. > > > > > --- > > > > > gcc/fold-const.c | 21 + > > > > > gcc/tree.c | 9 + > > > > > 2 files changed, 30 insertions(+) > > > > >
Re: [PATCH 3/9] operand_equal_p: add support for OBJ_TYPE_REF.
> > When we compare OBJ_TYPE_REF_TOKEN and OBJ_TYPE_REF_OBJECT > and they are equal, are there cases where types_same_for_odr returns false? OBJ_TYPE_REF_OBJECT is pointer to the instance, OBJ_TYPE_REF_TOKEN is index in the vtable or the type given by obj_ref_type_class. I guess one can do something like void *ptr; ... if (cond) ((class_a *)ptr)->firstvirtualmethod_of_class_a (); else ((class_b *)ptr)->firstvirtualmethod_of_class_b (); Here OBJECT will be always *ptr, TOKEN will be 0, but the actual virtual method is different. So merging this may lead to invalid devirtualization at least when the classes are anonymous namespace and we work out late in compilation that they are not derived. > > I guess we went over this some time ago when talking about value-numbering > of them. I realize the devirt machinery use the class type to get at the > virtual table but since we later expand simply OBJ_TPE_REF_EXPR _that_ > already has the result of the load from the vitual table. > > So - do we need to compare obj_type_ref_class at all? The worst thing > that could happen is that devirt no longer can devirtualize after the merging > but it should never result in "wrong" devirtualization? So, do we really > want to inhibit ICF of two equal functions that could be eventually > devirtualized both but into different direct calls? Can't this sort of thing I think it is like with the TBAA info. I would suggest first implement tests that preserve correctness of both TBAA and devirt annotations. Incrementally we can teach ICF to be agressive with -Os and merge anyway while dropping the metadata (removing OBJ_TYPE_REF or adjusting alias sets > happen anyway when you'd factor in IPA-CP and two identical functions > with indirect calls to a function argument but called with different constant > args? And should IPA-CP then not consider cloning after ICF merging > (not sure how ordering works here). I am not sure how those are realated. For sure IPA-CP can clone when it works out that different types are passed to the function and devirtualize to different calls. But this is bit different - it is about the type annotation we Honza > > Richard. > > > Honza > > > > > > + } > > > > > > > > > > 2019-07-24 Martin Liska > > > > > > > > * fold-const.c (operand_equal_p): Support OBJ_TYPE_REF. > > > > * tree.c (add_expr): Hash parts of OBJ_TYPE_REF. > > > > --- > > > > gcc/fold-const.c | 21 + > > > > gcc/tree.c | 9 + > > > > 2 files changed, 30 insertions(+) > > > >
Re: [PATCH 3/9] operand_equal_p: add support for OBJ_TYPE_REF.
On Thu, Aug 15, 2019 at 4:57 PM Jan Hubicka wrote:
>
> > On Tue, Aug 6, 2019 at 5:43 PM Martin Liska wrote:
> > >
> > >
> > > gcc/ChangeLog:
> >
> > + /* Virtual table call. */
> > + case OBJ_TYPE_REF:
> > + {
> > + if (!operand_equal_p (OBJ_TYPE_REF_EXPR (arg0),
> > + OBJ_TYPE_REF_EXPR (arg1), flags))
> > + return false;
> > + if (virtual_method_call_p (arg0))
> > + {
> > + if (tree_to_uhwi (OBJ_TYPE_REF_TOKEN (arg0))
> > + != tree_to_uhwi (OBJ_TYPE_REF_TOKEN (arg1)))
> > + return false;
> > + if (!types_same_for_odr (obj_type_ref_class (arg0),
> > +obj_type_ref_class (arg1)))
> > + return false;
> > + if (!operand_equal_p (OBJ_TYPE_REF_OBJECT (arg0),
> > + OBJ_TYPE_REF_OBJECT (arg1), flags))
> > + return false;
> >
> > this all gets deep into the devirt machinery, including looking at
> > ODR type hashes. So I'm not sure if we really want to handle
> > it this "optimistic" in operand_equal_p and completely ignore
> > other operands when !virtual_method_call_p? That is, why
> > not compare OBJ_TYPE_REF_TOKEN/OBJECT always at least?
>
> For !virtual_method_call_p we do not use OBJ_TYPE_REF at all yet obj-C
> frontend
> produce it. I think we should remove them somewhere during gimplification.
> We can definitly turn "optimistic" to "pesimistic" and return false here.
>
> Otherwise the checks makes sense to me - it the tests above passes devirt
> machinery ought to give same results.
> >
> > Do we then have cases where the OBJ_TYPE_REF is actually
> > distinct according to the remaining check?
>
> I am not sure what you mean here?
When we compare OBJ_TYPE_REF_TOKEN and OBJ_TYPE_REF_OBJECT
and they are equal, are there cases where types_same_for_odr returns false?
I guess we went over this some time ago when talking about value-numbering
of them. I realize the devirt machinery use the class type to get at the
virtual table but since we later expand simply OBJ_TPE_REF_EXPR _that_
already has the result of the load from the vitual table.
So - do we need to compare obj_type_ref_class at all? The worst thing
that could happen is that devirt no longer can devirtualize after the merging
but it should never result in "wrong" devirtualization? So, do we really
want to inhibit ICF of two equal functions that could be eventually
devirtualized both but into different direct calls? Can't this sort of thing
happen anyway when you'd factor in IPA-CP and two identical functions
with indirect calls to a function argument but called with different constant
args? And should IPA-CP then not consider cloning after ICF merging
(not sure how ordering works here).
Richard.
> Honza
> >
> > + }
> >
> >
> > > 2019-07-24 Martin Liska
> > >
> > > * fold-const.c (operand_equal_p): Support OBJ_TYPE_REF.
> > > * tree.c (add_expr): Hash parts of OBJ_TYPE_REF.
> > > ---
> > > gcc/fold-const.c | 21 +
> > > gcc/tree.c | 9 +
> > > 2 files changed, 30 insertions(+)
> > >
Re: [PATCH 3/9] operand_equal_p: add support for OBJ_TYPE_REF.
> On Tue, Aug 6, 2019 at 5:43 PM Martin Liska wrote:
> >
> >
> > gcc/ChangeLog:
>
> + /* Virtual table call. */
> + case OBJ_TYPE_REF:
> + {
> + if (!operand_equal_p (OBJ_TYPE_REF_EXPR (arg0),
> + OBJ_TYPE_REF_EXPR (arg1), flags))
> + return false;
> + if (virtual_method_call_p (arg0))
> + {
> + if (tree_to_uhwi (OBJ_TYPE_REF_TOKEN (arg0))
> + != tree_to_uhwi (OBJ_TYPE_REF_TOKEN (arg1)))
> + return false;
> + if (!types_same_for_odr (obj_type_ref_class (arg0),
> +obj_type_ref_class (arg1)))
> + return false;
> + if (!operand_equal_p (OBJ_TYPE_REF_OBJECT (arg0),
> + OBJ_TYPE_REF_OBJECT (arg1), flags))
> + return false;
>
> this all gets deep into the devirt machinery, including looking at
> ODR type hashes. So I'm not sure if we really want to handle
> it this "optimistic" in operand_equal_p and completely ignore
> other operands when !virtual_method_call_p? That is, why
> not compare OBJ_TYPE_REF_TOKEN/OBJECT always at least?
For !virtual_method_call_p we do not use OBJ_TYPE_REF at all yet obj-C frontend
produce it. I think we should remove them somewhere during gimplification.
We can definitly turn "optimistic" to "pesimistic" and return false here.
Otherwise the checks makes sense to me - it the tests above passes devirt
machinery ought to give same results.
>
> Do we then have cases where the OBJ_TYPE_REF is actually
> distinct according to the remaining check?
I am not sure what you mean here?
Honza
>
> + }
>
>
> > 2019-07-24 Martin Liska
> >
> > * fold-const.c (operand_equal_p): Support OBJ_TYPE_REF.
> > * tree.c (add_expr): Hash parts of OBJ_TYPE_REF.
> > ---
> > gcc/fold-const.c | 21 +
> > gcc/tree.c | 9 +
> > 2 files changed, 30 insertions(+)
> >
Re: [PATCH 3/9] operand_equal_p: add support for OBJ_TYPE_REF.
On Tue, Aug 6, 2019 at 5:43 PM Martin Liska wrote:
>
>
> gcc/ChangeLog:
+ /* Virtual table call. */
+ case OBJ_TYPE_REF:
+ {
+ if (!operand_equal_p (OBJ_TYPE_REF_EXPR (arg0),
+ OBJ_TYPE_REF_EXPR (arg1), flags))
+ return false;
+ if (virtual_method_call_p (arg0))
+ {
+ if (tree_to_uhwi (OBJ_TYPE_REF_TOKEN (arg0))
+ != tree_to_uhwi (OBJ_TYPE_REF_TOKEN (arg1)))
+ return false;
+ if (!types_same_for_odr (obj_type_ref_class (arg0),
+obj_type_ref_class (arg1)))
+ return false;
+ if (!operand_equal_p (OBJ_TYPE_REF_OBJECT (arg0),
+ OBJ_TYPE_REF_OBJECT (arg1), flags))
+ return false;
this all gets deep into the devirt machinery, including looking at
ODR type hashes. So I'm not sure if we really want to handle
it this "optimistic" in operand_equal_p and completely ignore
other operands when !virtual_method_call_p? That is, why
not compare OBJ_TYPE_REF_TOKEN/OBJECT always at least?
Do we then have cases where the OBJ_TYPE_REF is actually
distinct according to the remaining check?
+ }
> 2019-07-24 Martin Liska
>
> * fold-const.c (operand_equal_p): Support OBJ_TYPE_REF.
> * tree.c (add_expr): Hash parts of OBJ_TYPE_REF.
> ---
> gcc/fold-const.c | 21 +
> gcc/tree.c | 9 +
> 2 files changed, 30 insertions(+)
>
