This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Handle OBJ_TYPE_REF in FRE


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]