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.