[llvm-commits] turn on function merging

[Nick Lewycky]

Chris Lattner wrote:
>
> On Jul 11, 2010, at 12:12 AM, Nick Lewycky wrote:
>
>> I'd like to turn on the function merging pass by default (patch attached). Function merging is already in the tree and I've been testing it locally for a while. It works great and doesn't do undue harm to my build times.
>>
>> Functions are merged when they are identical under a comparison that treats all pointers as equivalent. So i8* and i32* are considered equivalent types, so long as you never load one which would produce an i8 as that's different from i32.
>
> Here is some general code review comments:
>
> Please comment the code, each method should get a function comment.  The linkage code needs comments explaining what is going on.
>
> Please rename 'hash' and 'fold' to something less generic, and give AliasGToF a longer name that says what it does.  The compare methods should be static or const.
>
> Stuff like this:
>
>      SmallVector<Value *, 8>  Indices1, Indices2;
>      for (GetElementPtrInst::const_op_iterator I = GEP1->idx_begin(),
>             E = GEP1->idx_end(); I != E; ++I) {
>        Indices1.push_back(*I);
>      }
>      for (GetElementPtrInst::const_op_iterator I = GEP2->idx_begin(),
>             E = GEP2->idx_end(); I != E; ++I) {
>        Indices2.push_back(*I);
>      }
>
> Should be able to be written as:
>
> SmallVector<Value *, 8>  Indices1(GEP1->idx_begin(), GEP1->idx_end());
> SmallVector<Value *, 8>  Indices2(GEP2->idx_begin(), GEP2->idx_end());
>
>
> I don't understand this logic:
> bool MergeFunctions::compare(const Value *V1, const Value *V2) {
>    if (V1 == LHS || V1 == RHS)
>      if (V2 == LHS || V2 == RHS)
>        return true;
>
> Also, related to that, making LHS and RHS be members of the MergeFunctions pass is kinda gross. It would be better to split the comparison stuff out to its own trivial comparison class, using a pattern like this:
>
> class FunctionComparisonator {
>    Function *LHS, *RHS;
> ...
> public:
>    FunctionComparisonator(...);
>
>    bool Compare();
> private:
> ...
> };
>
>    if (FunctionComparisonator(LHSFunc, RHSFunc).Compare()) ...
>
> That will also partition the comparison logic cleanly from the pass and transformation mechanics.
>
>
> This is some seriously crazy indentation, it hurts: :)
>
>      }
>      } break;
>    } break;
>
>
> It's unclear to me why you use "std::map<unsigned long, std::vector<Function *>  >  " as your hashing/uniquing data structure instead of a densemap.  Wouldn't it substantially simplify the code to use DenseMap<YourType>  where YourType just contains a Function* and a cached hash?  Then you just define your hash and equality predicates and the hash table does everything else?

I just tried doing that and the main problem I ran into is that I 
require TargetData to compare GEPs but the DenseMapInfo is entirely 
static. I don't see a way to include the TargetData* In the comparison, 
which is a show-stopper.

Possible fixes include making a ContextualDenseMap which takes a 
comparison object instead of a templated type, or making the GEP 
comparison really really good in the TargetData-less case.

> I'll take another look after this changes go in, thanks for working on this!

I've made substantial changes and would appreciate if you could revisit 
it now. Thanks for your review comments thus far!

Nick