[llvm-commits] [llvm] r94433 - in /llvm/trunk: include/llvm/Value.h lib/Transforms/Scalar/ScalarReplAggregates.cpp lib/VMCore/Value.cpp

[Bob Wilson]

On Jan 25, 2010, at 10:33 AM, Duncan Sands wrote:

> Hi Bob,
> 
>> Change Value::getUnderlyingObject to have the MaxLookup value specified as a
>> parameter with a default value, instead of just hardcoding it in the
>> implementation.  The limit of MaxLookup = 6 was introduced in r69151 to fix
>> a performance problem with O(n^2) behavior in instcombine, but the scalarrepl
>> pass is relying on getUnderlyingObject to go all the way back to an AllocaInst.
>> Making the limit part of the method signature makes it clear that by default
>> the result is limited and should help avoid similar problems in the future.
>> This fixes pr6126.
> 
> if sroa has to go back through a gazillion GEPS and bitcasts to get to the
> alloca, maybe it should just give up instead?

This is happening at a point where SROA has already decided to do the transformation.  It is just using getUnderlyingObject as a convenience.  Changing SROA to have a limit like that is possible, but it would involve more extensive changes.  We could also avoid using getUnderlyingObject by passing the alloca along as a parameter as the function recurses.  I'm not sure that's worth doing.

Regardless, I think it is good to have the MaxLookup value exposed as part of the getUnderlyingObject interface.  The header file documentation did not even say anything about there being a limit.