Re: Add few cases to operand_equal_p

[Richard Biener <rguenther at suse dot de>]

On Fri, 22 May 2015, Jan Hubicka wrote:

> > > +	case OBJ_TYPE_REF:
> > > +	  {
> > > +	    if (!operand_equal_p (OBJ_TYPE_REF_EXPR (arg0),
> > > +				  OBJ_TYPE_REF_EXPR (arg1), flags))
> > > +	      return false;
> > > +	    if (flag_devirtualize && 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;
> > 
> > devirt machinery in operand_equal_p?  please not.  not more places!
> 
> OBJ_TYPE_REF explicitly says what is the type of class whose virtual 
> method is called. It is GIMPLE operand expression like other, so I think 
> we need to handle it.
> 
> Actually I think I can just drop the flag_devirtualize check because with
> !flag_devirtualize we should simply avoid having OBJ_TYPE_REF around.  That
> would make it looking less devirt specific.  We can also test types for
> equivalence of main variant, but that would just introduce false positives when
> LTO did not merged two identical ODR types.  It would be correct though.

Ok, please split it out without the devritualize bits.

> > 
> > > +		/* OBJ_TYPE_REF_OBJECT is used to track the instance of
> > > + 		   object THIS pointer points to.  Checking that both
> > > +		   addresses equal is safe approximation of the fact
> > > +		   that dynamic types are equal.
> > > +		   Do not care about the other flags, because this expression
> > > +		   does not attribute to actual value of OBJ_TYPE_REF  */
> > > +		if (!operand_equal_p (OBJ_TYPE_REF_OBJECT (arg0),
> > > +				      OBJ_TYPE_REF_OBJECT (arg1),
> > > +				      OEP_ADDRESS_OF))
> > > +		  return false;
> > > +	      }
> > > +
> > > +	    return true;
> > > +	  }
> > > +
> > >  	default:
> > >  	  return 0;
> > >  	}
> > > @@ -3097,6 +3152,21 @@ operand_equal_p (const_tree arg0, const_
> > >  	}
> > >  
> > >      case tcc_declaration:
> > > +      /* At LTO time the FIELD_DECL may exist in multiple copies.
> > > + 	 We only care about offsets and bit offsets for operands.  */
> > 
> > Err?  Even at LTO time FIELD_DECLs should only appear once.
> > So - testcase?
> 
> FIELD_DECL has TREE_TYPE and TREE_TYPE may not get merged by LTO.  So if 
> the two expressions originate from two different units, we may have two 
> semantically equivalent FIELD_DECLs (of compatible types and same 
> offsets) that occupy different memory locations because their merging 
> was prevented by something upstream (like complete wrt incmplete pointer 
> in the type)

Still, operand_equal_p isn't about LTO, it's about GENERIC as created by
FEs...

> > > +      /* In GIMPLE empty constructors are allowed in initializers of
> > > + 	 vector types.  */
> > 
> > Why this comment about GIMPLE?  This is about comparing GENERIC
> > trees which of course can have CONSTRUCTORs of various sorts.
> > 
> > > +      if (TREE_CODE (arg0) == CONSTRUCTOR)
> > > +	{
> > > +	  unsigned length1 = vec_safe_length (CONSTRUCTOR_ELTS (arg0));
> > > +	  unsigned length2 = vec_safe_length (CONSTRUCTOR_ELTS (arg1));
> > > +
> > > +	  if (length1 != length2)
> > > +	    return false;
> > > +
> > > +	  flags &= ~(OEP_CONSTANT_ADDRESS_OF|OEP_ADDRESS_OF);
> > > +
> > > +	  for (unsigned i = 0; i < length1; i++)
> > > +	    if (!operand_equal_p (CONSTRUCTOR_ELT (arg0, i)->value,
> > > +				  CONSTRUCTOR_ELT (arg1, i)->value, flags))
> > 
> > You need to compare ->index as well here.  I'm not sure constructor
> > values are sorted always so you might get very many false negatives.
> 
> many false negatives is better than all false negatices :)
> 
> You are right, either I should punt on non-empty constructors or compare
> the indexes, will do the second.

Thanks - please split it out to a separate patch.

> > > +    case FIELD_DECL:
> > > +      /* At LTO time the FIELD_DECL may exist in multiple copies.
> > > +	 We only care about offsets and bit offsets for operands.  */
> > 
> > So explain that this is the reason we don't want to hash by
> > DECL_UID.  I still think this is bogus.
> 
> Will do if we agree on having this.
> 
> I know you would like ipa-icf to keep original bodies and use them for 
> inlining declaring alias sets to be function local.  This is wrong plan.  
> Consder:
> 
> void t(int *ptr)
> {
>   *ptr=1;
> }
> 
> int a(int *ptr1, int *ptr2)
> {
>   int a = *ptr1;
>   t(ptr2)
>   return a+*ptr1;
> }
> 
> long b(long *ptr1, int *ptr2)
> {
>   int a = *ptr1;
>   t(ptr2)
>   return a+*ptr1;
> }
> 
> here aliasing leads to the two options to be optimizer differently:
> a:
> .LFB1:  
>         .cfi_startproc
>         movl    4(%esp), %edx
>         movl    8(%esp), %ecx
>         movl    (%edx), %eax
>         movl    $1, (%ecx)
>         addl    (%edx), %eax
>         ret
>         .cfi_endproc
> b:
> .LFB2:  
>         .cfi_startproc
>         movl    4(%esp), %eax
>         movl    8(%esp), %edx
>         movl    (%eax), %eax
>         movl    $1, (%edx)
>         addl    %eax, %eax
>         ret
>         .cfi_endproc
> 
> however with -fno-early-inlining the functions look identical (modulo alias
> sets) at ipa-icf time.  If we merged a/b, we could get wrong code for a
> even though no inlining of a or b happens.

First of all the return types don't agree so the testcase is bogus.

> So either we match the alias sets or we need to verify that the alias sets
> permit precisely the same set of optimizations with taking possible inlining
> into account.

Hmm, but then what makes ICF of a and b _with_ early inlining fail with
-fno-tree-fre1?  The casts from *ptr1 to int in the 'long' case.

So I think I need to see a real testcase and then I'll show you
even with no inlining after ICF you get wrong-code thus it is a bug
in ICF ;)

> I also do not believe that TBAA should be function local.  I believe it is
> useful to propagate stuff interprocedurally, like ipa-prop could be able to
> propagate this:
> 
> long *ptr1;
> int *ptr2;
> t(int *ptr)
> {
>   return *ptr;
> }
> wrap(int *ptr)
> {
>  *ptr1=1;
> }
> call()
> {
>   return wrap (*ptr2);
> }
> 
> and we could have ipa-reference style pass that collect alias sets 
> read/written by a function and uses it during local optimization to 
> figure out if there is a true dependence between function call and 
> memory store.

Sure, but after ICF there is no IPA propagation...

Richard.

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)