Re: -fstrict-aliasing fixes 6/6: permit inlining of comdats

[Jan Hubicka <hubicka@ucw.cz>]

> 
> I wonder if you can split out the re-naming at this stage.  Further
> comments below.

OK, I will commit the renaming and ipa-icf fix separately.
> 
> > Bootstrapped/regtested x86_64-linux, OK?
> > 
> > I will work on some testcases for the ICF and fold-const that would lead
> > to wrong code if alias sets was ignored early.
> 
> Would be nice to have a wrong-code testcase go with the commit.
> 
> > Honza
> > 	* fold-const.c (operand_equal_p): Before inlining do not permit
> > 	transformations that would break with strict aliasing.
> > 	* ipa-inline.c (can_inline_edge_p) Use merged_comdat.
> > 	* ipa-inline-transform.c (inline_call): When inlining merged comdat do
> > 	not drop strict_aliasing flag of caller.
> > 	* cgraphclones.c (cgraph_node::create_clone): Use merged_comdat.
> > 	* cgraph.c (cgraph_node::dump): Dump merged_comdat.
> > 	* ipa-icf.c (sem_function::merge): Drop merged_comdat when merging
> > 	comdat and non-comdat.
> > 	* cgraph.h (cgraph_node): Rename merged to merged_comdat.
> > 	* ipa-inline-analysis.c (simple_edge_hints): Check both merged_comdat
> > 	and icf_merged.
> > 
> > 	* lto-symtab.c (lto_cgraph_replace_node): Update code computing
> > 	merged_comdat.
> > Index: fold-const.c
> > ===================================================================
> > --- fold-const.c	(revision 231239)
> > +++ fold-const.c	(working copy)
> > @@ -2987,7 +2987,7 @@ operand_equal_p (const_tree arg0, const_
> >  					   flags)))
> >  		return 0;
> >  	      /* Verify that accesses are TBAA compatible.  */
> > -	      if (flag_strict_aliasing
> > +	      if ((flag_strict_aliasing || !cfun->after_inlining)
> >  		  && (!alias_ptr_types_compatible_p
> >  		        (TREE_TYPE (TREE_OPERAND (arg0, 1)),
> >  		         TREE_TYPE (TREE_OPERAND (arg1, 1)))
> 
> Sooo....  first of all the code is broken anyway as it guards
> the restrict checking (MR_DEPENDENCE_*) stuff with flag_strict_aliasing
> (ick).  Second, I wouldn't mind if we drop the flag_strict_aliasing
> check alltogether, a cfun->after_inlining checks makes me just too
> nervous.

OK, I will drop the check separately, too.  
Next stage1 we need to look into code merging across alias classes. ipa-icf
scores are currently 40% down compared to GCC 5 at Firefox.
> 
> So your logic relies on the fact that the -fno-strict-aliasing was
> not necessary on copy A if copy B was compiled without that flag
> because otherwise copy B would invoke undefined behavior?

Yes.
> 
> This menans it's a language semantics thing but you simply look at
> whether it's "comdat"?  Shouldn't this use some ODR thing instead?

It is definition of COMDAT. COMDAT functions are output in every unit
used and no matter what body wins the linking is correct.  Only C++
produce comdat functions, so they all comply ODR rule, so we could rely
on the fact that all function bodies should be equivalent on a source
level.
> 
> Also as undefined behavior only applies at runtime consider copy A
> (with -fno-strict-aliasing) is used in contexts where undefined
> behavior would occur while copy B not.  Say,
> 
> int foo (int *p, short *q)
> {
>   *p = 1;
>   return *q;
> }
> 
> and the copy A use is foo (&x, &x) while the copy B use foo (&x, &y).
> 
> Yes, the case is lame here as we'd miscompile this in copy B and
> comdat makes us eventually use that copy for A.  But if we don't
> manage to miscompile this without inlining there isn't any undefined
> behavior (at runtime) you can rely on.

Well, it is ODR violation in this case :)
> 
> Just want to know whether you thought about the above cases, I would
> declare them invalid but I am not sure the C++ standard agrees here.

Well, not exactly of the case mentioned above, but still think that this is
safe (ugly, too). An alternative is to keep around the bodies until after
inlining.  I have infrastructure for that in my tree, but it is hard to tune to
do: first the alternative function body may have different symbol references
(as a result of different early inlinin) which may not be resolved in current
binary so we can not use it at all. Second keepin many alternatives of every
body around makes code size estimates in inliner go crazy.

Honza