On 07/17/2014 05:05 PM, Martin Liška wrote:
>
> On 07/06/2014 12:53 AM, Jan Hubicka wrote:
>>> On Fri, 20 Jun 2014, Trevor Saunders wrote:
>>>>> +@item -fipa-icf
>>>>> +@opindex fipa-icf
>>>>> +Perform Identical Code Folding for functions and read-only variables.
>> I would perhaps explicitly say that the optimizations reduce code size
>> and may disturb unwind stacks by replacing a function by equivalent
>> one with different name.
>>>>> +Behavior is similar to Gold Linker ICF optimization. Symbols proved
>> Perhaps tell a bit more here. The optimization works more effectively with link
>> time optimization enabled and that the Gold and GCC ICF works on different
>> levels and thus are not equivalent optimizations - there are equivallences that
>> are found only by GCC and equivalences found only by Gold.
>>
>>>> +as semantically equivalent are redirected to corresponding symbol. The pass
>>>>> +sensitively decides for usage of alias, thunk or local redirection.
>>>>> +This flag is enabled by default at @option{-O2}.
>> Probably at -Os too.
>>> I found this a bit hard to read/understand.
>>>
>>> Perhaps first describe what it does and then, before "This flag is
>>> enabled..." note that "This is similar to the ICF optimization performed
>>> by the Gold linker".
>>> "Symbols proved" (plural) vs "to corresponding symbol" seems to miss
>>> an an "a" as in "a corresponding symbol".  Alas, how is that one
>>> determined?  Is this more "...are merged into one", from the user's
>>> perspective?
>>>
>>> What does it mean to "sensitively decide for usage of alias, thunk,
>>> or local redirection"?
>> I think this is just a technical detail of the implementation.  I would not put that
>> into user manual.  It means that for some functions you can make alias, for others
>> you need thunk (so addresses stay different)
>>> Gerald
>
> Hello,
>     there's updated version of patch that newly uses devirtualization machinery to identify polymorphic types that can potentially break ICF (There are such examples in Firefox).
>
> Apart from that, I did many small updates, incorporated Trevor's comments and I tried to improve documentation entry for the pass.
> Patch has been tested for Firefox and Inkscape with LTO.
>
> Thanks,
> Martin

Hello.

After couple of weeks I spent with fixing new issues connected to the pass:
1) Inliner failed in case I created a thunk and release body of a function. In such situation we need to preserve DECL_ARGUMENTS. I added new argument for: cgraph_node::release_body.
2) Awkward error was hidden in libstdc++ test for trees, there were two functions having one argument that differs in one sub-template. Thank to Richard who helped me to fix alias set accuracy.
3) There was missing comparison for FIELD_DECLS (DECL_FIELD_BIT_OFFSET) which caused me miscompilation.
4) After discussion with Honza, we introduced new cgraph_node flag called icf_merged. The flag helps to fix verifier in cgraph_node::verify.

Current version of the patch can bootstrap on x86_64-linux. With following patch applied, there's not testcase regression.
I tried to build Firefox, Inkscape, GIMP and Chromium with LTO and patch applied and no regression has been observed.

Moreover, I discussed with Richard and the pass is capable of playing role in tree-ssa-tail-merge (according to first experiments). It can replace current usage of value numbering.

I hope we can apply the patch to the mainline in a short-term time window?

Thank you,
Martin