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

[Nick Lewycky]

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?

Nick