[llvm-commits] PATCH: Cleanup allocas aggressively in instcombine

[Chandler Carruth]

On Sun, Apr 8, 2012 at 1:32 AM, Chandler Carruth <chandlerc at gmail.com>wrote:

> On Sun, Apr 8, 2012 at 12:12 AM, Nick Lewycky <nicholas at mxc.ca> wrote:
>
>> Chandler Carruth wrote:
>> > Hello,
>> >
>> > One of the last places where LLVM doesn't generate good code for the
>> > hashing seems to be a pretty fundamental issue with cleanup during
>> > iterative SROA / inlining / instcombine progressions:
>> >
>> > We start out with an alloca, store values into it, and then call
>> > functions which operate on the alloca'ed memory. As we inline these
>> > functions (because they're trivial in this case), we iteratively
>> > propagate the values directly into the computation rather than using the
>> > alloca. However, we still need to store the values inte the alloca
>> > because of (often dead) code that is still using it. Eventually, we
>> > prove all the code using the alloca dead or we manage to push all the
>> > values directly into the computations.
>> >
>> > Unfortunately, by this time, the inliner and SROA passes have finished,
>> > and they finished while there were still uses of the alloca'ed pointer
>> > and so the stores weren't completely dead.
>> >
>> > This patch teaches instcombine to walk alloca instructions and if they
>> > are only used by GEPs, bitcasts, and stores, remove them. I added this
>> > to instcombine because I would like this to run iteratively with other
>> > passes so that dead allocas don't block further inlining, etc.
>> >
>> > Seem reasonable? I'm working on reduced test cases, I've been testing it
>> > against the hashing code, and this nicely removes the dead alloca
>> > instructions which is the only remaining cruft in the following code:
>> >
>> > hash_code foo(int *x, int *y) { return hash_combine(x, y); }
>>
>> +        // Simple stores into the alloca are only live if the alloca is
>> live.
>> +      case Instruction::Store:
>> +        if (!cast<StoreInst>(I)->isSimple())
>> +          return 0;
>> +        DeadStores.push_back(I);
>> +        continue;
>> +      }
>>
>> Firstly, indentation on the comment is more than the line below it.
>>
>> More importantly, I think isSimple is too strict, we should be able to
>> eliminate all non-volatile stores to allocas here.
>>
>> +  while (!DeadStores.empty())
>> +    IC.EraseInstFromFunction(*DeadStores.pop_back_val());
>>
>> What happens if you have a duplicate entry in DeadStores? Imagine you
>> have a store that stores the address of one dead alloca into another
>> dead alloca. Do you erase the store then access dead memory the second
>> time?
>
>
> Great points, the store logic wasn't sound in general. We should only
> process the store based on its pointer operand, not based on the other
> operands. That should ensure we only ever add the store to the list once.
>
> I've fixed these issues, and attached an updated patch. I included test
> cases both for the core functionality and for the cases you pointed out. =]
>

I'm going to go ahead and commit this (modulo some 80-columns fixes) as I
think I've covered your issues and Benjamin took a glance at it and it
seemed good to him too.

Thanks for the great review, lemme know if anything else jumps out at you.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120408/70982d1c/attachment.html>