[llvm-commits] [llvm] r91459 - in /llvm/trunk: lib/Transforms/Scalar/ScalarReplAggregates.cpp test/Transforms/ScalarRepl/2009-12-11-NeonTypes.ll

[Bob Wilson]

On Dec 17, 2009, at 2:22 PM, Chris Lattner wrote:

> 
> On Dec 15, 2009, at 2:00 PM, Bob Wilson wrote:
> 
>> Author: bwilson
>> Date: Tue Dec 15 16:00:51 2009
>> New Revision: 91459
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=91459&view=rev
>> Log:
>> Reapply 91184 with fixes and an addition to the testcase to cover the problem
>> found last time.  Instead of trying to modify the IR while iterating over it,
>> I've change it to keep a list of WeakVH references to dead instructions, and
>> then delete those instructions later.  I also added some special case code to
>> detect and handle the situation when both operands of a memcpy intrinsic are
>> referencing the same alloca.
> 
> This is a pretty big hammer.  Can you explain in what circumstance this happens?  The only case I can think of where this would occur is when one instruction uses two pointers derived from the same alloca.  This can only really happen for phis, calls, and yes, memset/cpy/move.
> 
> However, the only time that memset/cpy/move could do this is when they are a noop copy from the start of the alloca to the end of the alloca.  Is this the only case that you're interested in here?  If so, there are probably lighter weight ways to solve this problem.

The immediate problem that prompted this change was indeed the no-op memcpy.  But, while investigating it, I realized that I hadn't thought at all about the potential perils of modifying the instructions while iterating on their use lists.  Apparently I didn't think hard enough because the x86_64 self-host buildbot just hit a similar problem....

I assume the big hammer you're referring to is the use of the WeakVH references?  I introduced that with one of my earlier attempts at solving this problem, and I don't think it's even necessary anymore.  I will try to replace it.

The code for handling the no-op memcpy is actually very lightweight: In RewriteMemIntrinsicUserOfAlloca, I changed the code to remove bitcasts and all-zero GEPs from "OtherPtr" so that it iterates instead of just removing one bitcast and one GEP, and then after that loop, I added this check:

    // If OtherPtr has already been rewritten, this intrinsic will be dead.
    if (OtherPtr == NewElts[0])
      return;