<div class="gmail_quote">On Sun, Apr 8, 2012 at 12:12 AM, Nick Lewycky <span dir="ltr"><<a href="mailto:nicholas@mxc.ca">nicholas@mxc.ca</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">Chandler Carruth wrote:<br>
> Hello,<br>
><br>
> One of the last places where LLVM doesn't generate good code for the<br>
> hashing seems to be a pretty fundamental issue with cleanup during<br>
> iterative SROA / inlining / instcombine progressions:<br>
><br>
> We start out with an alloca, store values into it, and then call<br>
> functions which operate on the alloca'ed memory. As we inline these<br>
> functions (because they're trivial in this case), we iteratively<br>
> propagate the values directly into the computation rather than using the<br>
> alloca. However, we still need to store the values inte the alloca<br>
> because of (often dead) code that is still using it. Eventually, we<br>
> prove all the code using the alloca dead or we manage to push all the<br>
> values directly into the computations.<br>
><br>
> Unfortunately, by this time, the inliner and SROA passes have finished,<br>
> and they finished while there were still uses of the alloca'ed pointer<br>
> and so the stores weren't completely dead.<br>
><br>
> This patch teaches instcombine to walk alloca instructions and if they<br>
> are only used by GEPs, bitcasts, and stores, remove them. I added this<br>
> to instcombine because I would like this to run iteratively with other<br>
> passes so that dead allocas don't block further inlining, etc.<br>
><br>
> Seem reasonable? I'm working on reduced test cases, I've been testing it<br>
> against the hashing code, and this nicely removes the dead alloca<br>
> instructions which is the only remaining cruft in the following code:<br>
><br>
> hash_code foo(int *x, int *y) { return hash_combine(x, y); }<br>
<br>
</div></div>+        // Simple stores into the alloca are only live if the alloca is<br>
live.<br>
+      case Instruction::Store:<br>
+        if (!cast<StoreInst>(I)->isSimple())<br>
+          return 0;<br>
+        DeadStores.push_back(I);<br>
+        continue;<br>
+      }<br>
<br>
Firstly, indentation on the comment is more than the line below it.<br>
<br>
More importantly, I think isSimple is too strict, we should be able to<br>
eliminate all non-volatile stores to allocas here.<br>
<br>
+  while (!DeadStores.empty())<br>
+    IC.EraseInstFromFunction(*DeadStores.pop_back_val());<br>
<br>
What happens if you have a duplicate entry in DeadStores? Imagine you<br>
have a store that stores the address of one dead alloca into another<br>
dead alloca. Do you erase the store then access dead memory the second time?</blockquote><div><br></div><div>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.</div>
<div><br></div><div>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. =]</div></div>