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

[Nick Lewycky]

Bob Wilson wrote:
>
> 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.

If that's a 1-minute change to make then please do it. It will save 
someone grief when trying to figure out why SROA is slow on their input 
testcase some day.

> 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.

Sure.

Nick