On Fri, 4 Dec 2015, Jan Hubicka wrote:
> >
> > 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.
/me inserts his usual "partitioning to the rescue" comment...
;)
Richard.
