<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Dec 29, 2016 at 1:23 AM, Piotr Padlewski <span dir="ltr"><<a href="mailto:piotr.padlewski@gmail.com" target="_blank">piotr.padlewski@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">CurrIter have only one use. <div><br></div><div><br></div><div><span style="color:rgb(80,0,80);font-size:12.8px">auto CurrIter = MI;</span><br style="color:rgb(80,0,80);font-size:12.8px"><span style="color:rgb(80,0,80);font-size:12.8px">++MI;</span><br style="color:rgb(80,0,80);font-size:12.8px"><span style="color:rgb(80,0,80);font-size:12.8px">Value *Member = *CurrIter;</span><br></div><div><span style="color:rgb(80,0,80);font-size:12.8px"><br></span></div><div>I can rewrite beginning of loop as<span style="color:rgb(80,0,80);font-size:12.8px"><br></span></div><div><span style="color:rgb(80,0,80);font-size:12.8px">Value *Member = *MI++;</span></div><div><span style="color:rgb(80,0,80);font-size:12.8px"><br></span></div><div><span style="color:rgb(80,0,80);font-size:12.8px">Which makes it trivial that it is range for loop.</span></div><div><span style="color:rgb(80,0,80);font-size:12.8px"><br></span></div><div><span style="color:rgb(80,0,80);font-size:12.8px">Of course I am not 100% sure, operator* and operator++ might do some weird things here, but I assumed they are sane.</span></div><div><span style="color:rgb(80,0,80);font-size:12.8px">Tests also passed</span></div></div></blockquote><div><br></div><div><br></div><div>The pattern I'm thinking of is something like iterating the instructions in a basic block, then calling eraseFromParent on the current Instruction, which will nonetheless corrupt the list even if the iterator itself has only one use. That's not the case here though. This particular code is safe because the mutation of the thing being iterated is done after the loop (the `CC->Members.swap(MembersLeft);` call); i.e. a new container is allocated and then they are swapped at the end, instead of mutating in place.</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><span style="color:rgb(80,0,80);font-size:12.8px"><br></span></div></div><div class="gmail-HOEnZb"><div class="gmail-h5"><div class="gmail_extra"><br><div class="gmail_quote">2016-12-29 2:20 GMT+01:00 Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Wed, Dec 28, 2016 at 11:42 AM, Piotr Padlewski via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: prazek<br>
Date: Wed Dec 28 13:42:49 2016<br>
New Revision: 290683<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=290683&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=290683&view=rev</a><br>
Log:<br>
[NewGVN] Simplyfy loop NFC<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/Scal<wbr>ar/NewGVN.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/Scal<wbr>ar/NewGVN.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp?rev=290683&r1=290682&r2=290683&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/lib/Transform<wbr>s/Scalar/NewGVN.cpp?rev=290683<wbr>&r1=290682&r2=290683&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/lib/Transforms/Scal<wbr>ar/NewGVN.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Scal<wbr>ar/NewGVN.cpp Wed Dec 28 13:42:49 2016<br>
@@ -1915,10 +1915,7 @@ bool NewGVN::eliminateInstructions(<wbr>Funct<br>
<br>
// Cleanup the congruence class.<br>
SmallPtrSet<Value *, 4> MembersLeft;<br>
- for (auto MI = CC->Members.begin(), ME = CC->Members.end(); MI != ME;) {<br>
- auto CurrIter = MI;<br>
- ++MI;<br>
- Value *Member = *CurrIter;<br>
+ for (Value * Member : CC->Members) {<br></blockquote><div><br></div></span><div>Is this correct? Usually this kind of loop where the iterator is incremented at the start of the loop will invalidate the iterator somewhere later, so rewriting it to be a range for (or a simple loop with the increment inside the `for` header where it usually goes) will introduce a nasty bug.</div><div><br></div><div>I haven't looked closely enough in this particular case, but I'd definitely like to see a bit more justification (ideally in the commit message in the future) for the safety of this refactoring when dealing with this kind of "increment at the beginning" loops.</div><span class="gmail-m_548872020588823726HOEnZb"><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
if (Member->getType()->isVoidTy()<wbr>) {<br>
MembersLeft.insert(Member);<br>
continue;<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>