[llvm-commits] [llvm] r94433 - in /llvm/trunk: include/llvm/Value.h lib/Transforms/Scalar/ScalarReplAggregates.cpp lib/VMCore/Value.cpp
nicholas at mxc.ca
Mon Jan 25 21:56:47 PST 2010
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.
More information about the llvm-commits