This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Handle OBJ_TYPE_REF in FRE
- From: Richard Biener <rguenther at suse dot de>
- To: Jan Hubicka <hubicka at ucw dot cz>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Fri, 4 Dec 2015 10:01:51 +0100 (CET)
- Subject: Re: [PATCH] Handle OBJ_TYPE_REF in FRE
- Authentication-results: sourceware.org; auth=none
- References: <alpine dot LSU dot 2 dot 11 dot 1512031142360 dot 4884 at t29 dot fhfr dot qr> <20151203174007 dot GA59701 at kam dot mff dot cuni dot cz> <133CDD55-7EFC-41F9-81C1-BEC5717A9984 at suse dot de> <20151203225233 dot GA56018 at kam dot mff dot cuni dot cz>
On Thu, 3 Dec 2015, Jan Hubicka wrote:
> > >may lead to wrong code.
> >
> > Can you try generating a testcase?
> > Because with equal vptr and voffset I can't see how that can happen
> > unless some pass extracts information from the pointer types without
> > sanity checking with the pointers and offsets.
>
> I am not sure I can get a wrong code with current mainline, because for now you
> only substitute for the lookup done for speculative devirt and if we wrongly
> predict the thing to be __builtin_unreachable, we dispatch to usual virtual
> call. Once you get movement on calls it will be easier to do.
>
> OBJ_TYPE_REF is a wrapper around OBJ_TYPE_EXPR adding three extra parameters:
> - OBJ_TYPE_REF_OBJECT
> - OBJ_TYPE_REF_TOKEN
> - obj_type_ref_class which is computed from TREE_TYPE (obj_type_ref) itself.
>
> While two OBJ_TYPE_REFS with equivalent OBJ_TYPE_EXPR are kind of same
> expressions, they are optimized differently (just as if they was in different
> alias set). For that reason you need to match the type of obj_type_ref_class
> because that one is not matched by usless_type_conversion (it is a pointer to
> method of corresponding class type we are looking up)
>
> The following testcase:
> struct foo {virtual void bar(void) __attribute__ ((const));};
> struct foobar {virtual void bar(void) __attribute__ ((const));};
> void
> dojob(void *ptr, int t)
> {
> if (t)
> ((struct foo*)ptr)->bar();
> else
> ((struct foobar*)ptr)->bar();
> }
>
> produces
> void dojob(void*, int) (void * ptr, int t)
> {
> int (*__vtbl_ptr_type) () * _5;
> int (*__vtbl_ptr_type) () _6;
> int (*__vtbl_ptr_type) () * _8;
> int (*__vtbl_ptr_type) () _9;
>
> <bb 2>:
> if (t_2(D) != 0)
> goto <bb 3>;
> else
> goto <bb 4>;
>
> <bb 3>:
> _5 = MEM[(struct foo *)ptr_4(D)]._vptr.foo;
> _6 = *_5;
> OBJ_TYPE_REF(_6;(struct foo)ptr_4(D)->0) (ptr_4(D));
> goto <bb 5>;
>
> <bb 4>:
> _8 = MEM[(struct foobar *)ptr_4(D)]._vptr.foobar;
> _9 = *_8;
> OBJ_TYPE_REF(_9;(struct foobar)ptr_4(D)->0) (ptr_4(D));
>
> <bb 5>:
> return;
>
> }
>
> Now I would need to get some code movement done to get _5 and _6
> moved and unified with _8 and _9 that we currently don't do.
> Still would feel safer if the equivalence predicate also checked
> that the type is the same.
Indeed we don't do code hoisting yet. Maybe one could trick PPRE
into doing it.
Note that for OBJ_TYPE_REFs in calls you probably should better use
gimple_call_fntype instead of the type of the OBJ_TYPE_REF anyway
(well, fntype will be the method-type, not pointer-to-method-type).
Not sure if you need OBJ_TYPE_REFs type in non-call contexts?
> > >Or do you just substitute the operands of OBJ_TYPE_REF?
> >
> > No, I value number them. But yes, the type issue also crossed my
> > mind. Meanwhile testing revealed that I need to adjust
> > gimple_expr_type to preserve the type of the obj-type-ref, otherwise
> > the devirt machinery ICEs (receiving void *). That's also a reason we
> > can't make obj-type-ref a ternary RHS.
>
> Yep, type of OBJ_TYPE_REF matters...
See above.
> >
> > >> Bootstrap & regtest running on x86_64-unknown-linux-gnu.
> > >>
> > >> Note that this does not (yet) substitute OBJ_TYPE_REFs in calls
> > >> with SSA names that have the same value - not sure if that would
> > >> be desired generally (does the devirt machinery cope with that?).
> > >
> > >This should work fine.
> >
> > OK. So with that substituting the direct call later should work as well.
> Great!
For the above reasons I'm defering all this to stage1.
Below is the patch that actually passed bootstrap & regtest on
x86_64-unknown-linux-gnu, just in case you want to play with it.
It doesn't do the propagation into calls yet though, the following
does (untested)
Index: gcc/tree-ssa-pre.c
===================================================================
--- gcc/tree-ssa-pre.c (revision 231244)
+++ gcc/tree-ssa-pre.c (working copy)
@@ -4334,6 +4334,22 @@ eliminate_dom_walker::before_dom_childre
maybe_remove_unused_call_args (cfun, call_stmt);
gimple_set_modified (stmt, true);
}
+
+ else
+ {
+ /* Lookup the OBJ_TYPE_REF. */
+ tree sprime
+ = vn_nary_op_lookup_pieces (3, OBJ_TYPE_REF,
+ TREE_TYPE (fn),
+ &TREE_OPERAND (fn, 0),
NULL);
+ if (sprime)
+ sprime = eliminate_avail (sprime);
+ if (sprime)
+ {
+ gimple_call_set_fn (call_stmt, sprime);
+ gimple_set_modified (stmt, true);
+ }
+ }
}
}
but it ICEs because we decided (tree-cfg.c, verify_gimple_call):
if (fn
&& (!POINTER_TYPE_P (TREE_TYPE (fn))
|| (TREE_CODE (TREE_TYPE (TREE_TYPE (fn))) != FUNCTION_TYPE
&& TREE_CODE (TREE_TYPE (TREE_TYPE (fn))) != METHOD_TYPE)))
{
error ("non-function in gimple call");
return true;
}
and in useless_type_conversion_p:
/* Do not lose casts to function pointer types. */
if ((TREE_CODE (TREE_TYPE (outer_type)) == FUNCTION_TYPE
|| TREE_CODE (TREE_TYPE (outer_type)) == METHOD_TYPE)
&& !(TREE_CODE (TREE_TYPE (inner_type)) == FUNCTION_TYPE
|| TREE_CODE (TREE_TYPE (inner_type)) == METHOD_TYPE))
return false;
probably from the times we didn't have gimple_call_fntype. So if I
paper over the ICE (in the verifier) then the libreoffice testcase
gets optimized to
<bb 2>:
_3 = this_2(D)->D.2399.D.2325._vptr.B;
_4 = *_3;
PROF_6 = OBJ_TYPE_REF(_4;(struct
WindowListenerMultiplexer)this_2(D)->0);
if (PROF_6 == acquire)
goto <bb 3>;
else
goto <bb 4>;
<bb 3>:
PROF_6 (this_2(D));
goto <bb 5>;
<bb 4>:
PROF_6 (this_2(D));
by FRE2 and either VRP or DOM will propagate the equivalency to
<bb 2>:
_3 = this_2(D)->D.2399.D.2325._vptr.B;
_4 = *_3;
PROF_6 = OBJ_TYPE_REF(_4;(struct
WindowListenerMultiplexer)this_2(D)->0);
if (PROF_6 == acquire)
goto <bb 3>;
else
goto <bb 4>;
<bb 3>:
WindowListenerMultiplexer::acquire (this_2(D));
goto <bb 5>;
<bb 4>:
PROF_6 (this_2(D));
Richard.
2015-12-03 Richard Biener <rguenther@suse.de>
PR tree-optimization/64812
* tree-ssa-sccvn.c (vn_get_stmt_kind): Handle OBJ_TYPE_REF.
(vn_nary_length_from_stmt): Likewise.
(init_vn_nary_op_from_stmt): Likewise.
* gimple-match-head.c (maybe_build_generic_op): Likewise.
* gimple-pretty-print.c (dump_unary_rhs): Likewise.
* gimple-fold.c (gimple_build): Likewise.
* gimple.h (gimple_expr_type): Likewise.
* g++.dg/tree-ssa/ssa-fre-1.C: New testcase.
Index: gcc/tree-ssa-sccvn.c
===================================================================
*** gcc/tree-ssa-sccvn.c (revision 231221)
--- gcc/tree-ssa-sccvn.c (working copy)
*************** vn_get_stmt_kind (gimple *stmt)
*** 460,465 ****
--- 460,467 ----
? VN_CONSTANT : VN_REFERENCE);
else if (code == CONSTRUCTOR)
return VN_NARY;
+ else if (code == OBJ_TYPE_REF)
+ return VN_NARY;
return VN_NONE;
}
default:
*************** vn_nary_length_from_stmt (gimple *stmt)
*** 2479,2484 ****
--- 2481,2487 ----
return 1;
case BIT_FIELD_REF:
+ case OBJ_TYPE_REF:
return 3;
case CONSTRUCTOR:
*************** init_vn_nary_op_from_stmt (vn_nary_op_t
*** 2508,2513 ****
--- 2511,2517 ----
break;
case BIT_FIELD_REF:
+ case OBJ_TYPE_REF:
vno->length = 3;
vno->op[0] = TREE_OPERAND (gimple_assign_rhs1 (stmt), 0);
vno->op[1] = TREE_OPERAND (gimple_assign_rhs1 (stmt), 1);
Index: gcc/gimple-match-head.c
===================================================================
*** gcc/gimple-match-head.c (revision 231221)
--- gcc/gimple-match-head.c (working copy)
*************** maybe_build_generic_op (enum tree_code c
*** 243,248 ****
--- 243,249 ----
*op0 = build1 (code, type, *op0);
break;
case BIT_FIELD_REF:
+ case OBJ_TYPE_REF:
*op0 = build3 (code, type, *op0, op1, op2);
break;
default:;
Index: gcc/gimple-pretty-print.c
===================================================================
*** gcc/gimple-pretty-print.c (revision 231221)
--- gcc/gimple-pretty-print.c (working copy)
*************** dump_unary_rhs (pretty_printer *buffer,
*** 302,308 ****
|| TREE_CODE_CLASS (rhs_code) == tcc_reference
|| rhs_code == SSA_NAME
|| rhs_code == ADDR_EXPR
! || rhs_code == CONSTRUCTOR)
{
dump_generic_node (buffer, rhs, spc, flags, false);
break;
--- 302,309 ----
|| TREE_CODE_CLASS (rhs_code) == tcc_reference
|| rhs_code == SSA_NAME
|| rhs_code == ADDR_EXPR
! || rhs_code == CONSTRUCTOR
! || rhs_code == OBJ_TYPE_REF)
{
dump_generic_node (buffer, rhs, spc, flags, false);
break;
Index: gcc/testsuite/g++.dg/tree-ssa/ssa-fre-1.C
===================================================================
*** gcc/testsuite/g++.dg/tree-ssa/ssa-fre-1.C (revision 0)
--- gcc/testsuite/g++.dg/tree-ssa/ssa-fre-1.C (working copy)
***************
*** 0 ****
--- 1,44 ----
+ /* { dg-do compile } */
+ /* { dg-options "-O2 -fdump-tree-fre2" } */
+
+ template <class T> class A
+ {
+ T *p;
+
+ public:
+ A (T *p1) : p (p1) { p->acquire (); }
+ };
+
+ class B
+ {
+ public:
+ virtual void acquire ();
+ };
+ class D : public B
+ {
+ };
+ class F : B
+ {
+ int mrContext;
+ };
+ class WindowListenerMultiplexer : F, public D
+ {
+ void acquire () { acquire (); }
+ };
+ class C
+ {
+ void createPeer () throw ();
+ WindowListenerMultiplexer maWindowListeners;
+ };
+ class FmXGridPeer
+ {
+ public:
+ void addWindowListener (A<D>);
+ } a;
+ void
+ C::createPeer () throw ()
+ {
+ a.addWindowListener (&maWindowListeners);
+ }
+
+ /* { dg-final { scan-tree-dump-times "= OBJ_TYPE_REF" 1 "fre2" } } */
Index: gcc/gimple-fold.c
===================================================================
--- gcc/gimple-fold.c (revision 231221)
+++ gcc/gimple-fold.c (working copy)
@@ -6038,7 +6038,8 @@ gimple_build (gimple_seq *seq, location_
else
res = create_tmp_reg (type);
gimple *stmt;
- if (code == BIT_FIELD_REF)
+ if (code == BIT_FIELD_REF
+ || code == OBJ_TYPE_REF)
stmt = gimple_build_assign (res, code,
build3 (code, type, op0, op1, op2));
else
Index: gcc/gimple.h
===================================================================
--- gcc/gimple.h (revision 231221)
+++ gcc/gimple.h (working copy)
@@ -6079,7 +6079,9 @@ gimple_expr_type (const gimple *stmt)
}
else if (code == GIMPLE_ASSIGN)
{
- if (gimple_assign_rhs_code (stmt) == POINTER_PLUS_EXPR)
+ enum tree_code rcode = gimple_assign_rhs_code (stmt);
+ if (rcode == POINTER_PLUS_EXPR
+ || rcode == OBJ_TYPE_REF)
return TREE_TYPE (gimple_assign_rhs1 (stmt));
else
/* As fallback use the type of the LHS. */