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 <rguent...@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. */