Re: [PATCH 3/5] IPA ICF pass

[Jeff Law <law@redhat.com>]

On 06/26/14 12:46, Jan Hubicka wrote:
> So you've added this at -O2, what is the general compile-time
>> impact? Would it make more sense to instead have it be part of -O3,
>> particularly since ICF is rarely going to improve performance (sans
>> icache issues).
>
> I think code size optimization not sacrifying any (or too much of) performance are
> generally very welcome at -O2.  Compared to LLVM and Microsoft compilers we are
> on code bloat side at -O2.
I'm not so much worried about runtime performance here, but compile-time 
performance.  ICF would seem like a general win as we're likely going to 
be more icache efficient.

So, just to be clear, if the compile-time impact is minimal, then I'll 
fully support -O2, but my worry is that it won't be minimal :(


>> Is returning TRUE really the conservatively correct thing to do in
>> the absence of aliasing information?  Isn't that case really "I
>> don't know" in which case the proper return value is FALSE?
>
> I think with -fno-strict-aliasing the set should be 0 (Richi?) and thus we can
> compare for equality.  We probably can be on agressive side and let 0 alias
> set prevail the non-0.  But that can be done incrementally.
I'd think it should be zero in the -fno-strict-aliasing case.  My 
concern was that in the -fno-strict-aliasing case we seems to always 
assume the objects are the same.  Is that the safe thing to do?

That hints that the comment for the function needs tweaking.  It really 
doesn't say anything about what the return values really mean.


> There are few, like we can ignore "weak" or "visibility" attribute because we do
> produce alias with proper visibility anyway.  My plan is to start removing those
> attributes from declarations once they are turned into suitable representation
> in symbol table (or for attributes like const/noreturn/pure where we have explicit
> decl flags).  This will make our life bit easier later, too.
>
> We probably then can whitelist some attributes, but I would deal with this later.
Sure, I don't mind going with the conservative approach and iterating to 
remove some of the limitations.

>
> Yep, there are no resonable orders on it.  If function starts same in source code they ought
> to end up same here.  Plan was to first match for exact equality and then play with
> smarter tricks here.
Yea, that's fine too.  It's conservatively correct and we may find that 
there just isn't much to gain by doing hte dfs walk to build indices and 
such for the CFG.  I guess ultimately I just want someone to look at 
that issue and evaluate if what we're doing now is "good enough" or if 
we're missing most of the benefit because of something like bb index 
stability or something dumb like that.

>
> Yep, the pass has grown up to be rather long. The gimple equality testing is the
> main body of work, so perhaps doing this in separate file is good idea.
I'd certainly like to see that happen both because of its size and 
because I think those bits are useful in other contexts.

Overall, most of the stuff looks quite reasonable.

jeff