<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jun 7, 2017 at 3:45 PM, Alina Sbirlea <span dir="ltr"><<a href="mailto:alina.sbirlea@gmail.com" target="_blank">alina.sbirlea@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Could you clarify what to assert for?<div><br></div></div></blockquote><div>Yeah, i wasn't thinking straight.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div>If there is a definition prior to the current one in this block, it returns that one (&*Iter) - no point in asserting.</div></div></div></blockquote><div><br></div><div>Well the original point of asserting at all was that updater is mucking with the lists as it does this, but yes, it wasn't doing a lot :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div>Otherwise, it falls through and will return nullptr. I'm assuming assert before the fall through, where Iter == Defs->rend(), but the if condition is checking this (and obviously, dereferencing like in the original assert will crash).</div></div><div>I'm a bit confused...more details please?</div><div><br></div></div></blockquote><div>Just ignore me :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div></div><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Wed, Jun 7, 2017 at 1:58 PM, Daniel Berlin 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></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">Could you move the original assert to the def list case?<div class="m_-3617286185482380744HOEnZb"><div class="m_-3617286185482380744h5"><div><br><div class="gmail_quote"><div dir="ltr">On Wed, Jun 7, 2017, 9:47 AM Alina Sbirlea via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This revision was automatically updated to reflect the committed changes.<br>
Closed by commit rL304926: [mssa] Fix case when there is no definition in a block prior to an inserted use. (authored by asbirlea).<br>
<br>
Changed prior to commit:<br>
<a href="https://reviews.llvm.org/D33950?vs=101625&id=101760#toc" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3395<wbr>0?vs=101625&id=101760#toc</a><br>
<br>
Repository:<br>
rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D33950" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3395<wbr>0</a><br>
<br>
Files:<br>
llvm/trunk/lib/Analysis/Memory<wbr>SSAUpdater.cpp<br>
llvm/trunk/unittests/Analysis/<wbr>MemorySSA.cpp<br>
<br>
<br>
Index: llvm/trunk/lib/Analysis/Memory<wbr>SSAUpdater.cpp<br>
==============================<wbr>==============================<wbr>=======<br>
--- llvm/trunk/lib/Analysis/Memory<wbr>SSAUpdater.cpp<br>
+++ llvm/trunk/lib/Analysis/Memory<wbr>SSAUpdater.cpp<br>
@@ -124,17 +124,12 @@<br>
return &*Iter;<br>
} else {<br>
// Otherwise, have to walk the all access iterator.<br>
- auto Iter = MA->getReverseIterator();<br>
- ++Iter;<br>
- while (&*Iter != &*Defs->begin()) {<br>
- if (!isa<MemoryUse>(*Iter))<br>
- return &*Iter;<br>
- --Iter;<br>
- }<br>
- // At this point it must be pointing at firstdef<br>
- assert(&*Iter == &*Defs->begin() &&<br>
- "Should have hit first def walking backwards");<br>
- return &*Iter;<br>
+ auto End = MSSA->getWritableBlockAccesses<wbr>(MA->getBlock())->rend();<br>
+ for (auto &U : make_range(++MA->getReverseIte<wbr>rator(), End))<br>
+ if (!isa<MemoryUse>(U))<br>
+ return cast<MemoryAccess>(&U);<br>
+ // Note that if MA comes before Defs->begin(), we won't hit a def.<br>
+ return nullptr;<br>
}<br>
}<br>
return nullptr;<br>
Index: llvm/trunk/unittests/Analysis/<wbr>MemorySSA.cpp<br>
==============================<wbr>==============================<wbr>=======<br>
--- llvm/trunk/unittests/Analysis/<wbr>MemorySSA.cpp<br>
+++ llvm/trunk/unittests/Analysis/<wbr>MemorySSA.cpp<br>
@@ -244,6 +244,52 @@<br>
MSSA.verifyMemorySSA();<br>
}<br>
<br>
+TEST_F(MemorySSATest, SinkLoad) {<br>
+ F = Function::Create(<br>
+ FunctionType::get(B.getVoidTy(<wbr>), {B.getInt8PtrTy()}, false),<br>
+ GlobalValue::ExternalLinkage, "F", &M);<br>
+ BasicBlock *Entry(BasicBlock::Create(C, "", F));<br>
+ BasicBlock *Left(BasicBlock::Create(C, "", F));<br>
+ BasicBlock *Right(BasicBlock::Create(C, "", F));<br>
+ BasicBlock *Merge(BasicBlock::Create(C, "", F));<br>
+ B.SetInsertPoint(Entry);<br>
+ B.CreateCondBr(B.getTrue(), Left, Right);<br>
+ B.SetInsertPoint(Left, Left->begin());<br>
+ Argument *PointerArg = &*F->arg_begin();<br>
+ B.SetInsertPoint(Left);<br>
+ B.CreateBr(Merge);<br>
+ B.SetInsertPoint(Right);<br>
+ B.CreateBr(Merge);<br>
+<br>
+ // Load in left block<br>
+ B.SetInsertPoint(Left, Left->begin());<br>
+ LoadInst *LoadInst1 = B.CreateLoad(PointerArg);<br>
+ // Store in merge block<br>
+ B.SetInsertPoint(Merge, Merge->begin());<br>
+ B.CreateStore(B.getInt8(16), PointerArg);<br>
+<br>
+ setupAnalyses();<br>
+ MemorySSA &MSSA = *Analyses->MSSA;<br>
+ MemorySSAUpdater Updater(&MSSA);<br>
+<br>
+ // Mimic sinking of a load:<br>
+ // - clone load<br>
+ // - insert in "exit" block<br>
+ // - insert in mssa<br>
+ // - remove from original block<br>
+<br>
+ LoadInst *LoadInstClone = cast<LoadInst>(LoadInst1->clon<wbr>e());<br>
+ Merge->getInstList().insert(Me<wbr>rge->begin(), LoadInstClone);<br>
+ MemoryAccess * NewLoadAccess =<br>
+ Updater.createMemoryAccessInBB<wbr>(LoadInstClone, nullptr,<br>
+ LoadInstClone->getParent(),<br>
+ MemorySSA::Beginning);<br>
+ Updater.insertUse(cast<MemoryU<wbr>se>(NewLoadAccess));<br>
+ MSSA.verifyMemorySSA();<br>
+ Updater.removeMemoryAccess(MSS<wbr>A.getMemoryAccess(LoadInst1));<br>
+ MSSA.verifyMemorySSA();<br>
+}<br>
+<br>
TEST_F(MemorySSATest, MoveAStore) {<br>
// We create a diamond where there is a in the entry, a store on one side, and<br>
// a load at the end. After building MemorySSA, we test updating by moving<br>
<br>
<br>
</blockquote></div></div>
</div></div><br></div></div>______________________________<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>
<br></blockquote></div><br></div>
</blockquote></div><br></div></div>