[PATCH] D33950: [mssa] If there is no definition in a block prior to an inserted use, return nullptr.

[Daniel Berlin via llvm-commits]

On Wed, Jun 7, 2017 at 3:45 PM, Alina Sbirlea <alina.sbirlea at gmail.com>
wrote:

> Could you clarify what to assert for?
>
> Yeah, i wasn't thinking straight.


> If there is a definition prior to the current one in this block, it
> returns that one (&*Iter) - no point in asserting.
>

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 :)


> 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).
> I'm a bit confused...more details please?
>
> Just ignore me :)


>
> On Wed, Jun 7, 2017 at 1:58 PM, Daniel Berlin via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Could you move the original assert to the def list case?
>>
>> On Wed, Jun 7, 2017, 9:47 AM Alina Sbirlea via Phabricator <
>> reviews at reviews.llvm.org> wrote:
>>
>>> This revision was automatically updated to reflect the committed changes.
>>> Closed by commit rL304926: [mssa] Fix case when there is no definition
>>> in a block prior to an inserted use. (authored by asbirlea).
>>>
>>> Changed prior to commit:
>>>   https://reviews.llvm.org/D33950?vs=101625&id=101760#toc
>>>
>>> Repository:
>>>   rL LLVM
>>>
>>> https://reviews.llvm.org/D33950
>>>
>>> Files:
>>>   llvm/trunk/lib/Analysis/MemorySSAUpdater.cpp
>>>   llvm/trunk/unittests/Analysis/MemorySSA.cpp
>>>
>>>
>>> Index: llvm/trunk/lib/Analysis/MemorySSAUpdater.cpp
>>> ===================================================================
>>> --- llvm/trunk/lib/Analysis/MemorySSAUpdater.cpp
>>> +++ llvm/trunk/lib/Analysis/MemorySSAUpdater.cpp
>>> @@ -124,17 +124,12 @@
>>>          return &*Iter;
>>>      } else {
>>>        // Otherwise, have to walk the all access iterator.
>>> -      auto Iter = MA->getReverseIterator();
>>> -      ++Iter;
>>> -      while (&*Iter != &*Defs->begin()) {
>>> -        if (!isa<MemoryUse>(*Iter))
>>> -          return &*Iter;
>>> -        --Iter;
>>> -      }
>>> -      // At this point it must be pointing at firstdef
>>> -      assert(&*Iter == &*Defs->begin() &&
>>> -             "Should have hit first def walking backwards");
>>> -      return &*Iter;
>>> +      auto End = MSSA->getWritableBlockAccesses
>>> (MA->getBlock())->rend();
>>> +      for (auto &U : make_range(++MA->getReverseIterator(), End))
>>> +        if (!isa<MemoryUse>(U))
>>> +          return cast<MemoryAccess>(&U);
>>> +      // Note that if MA comes before Defs->begin(), we won't hit a def.
>>> +      return nullptr;
>>>      }
>>>    }
>>>    return nullptr;
>>> Index: llvm/trunk/unittests/Analysis/MemorySSA.cpp
>>> ===================================================================
>>> --- llvm/trunk/unittests/Analysis/MemorySSA.cpp
>>> +++ llvm/trunk/unittests/Analysis/MemorySSA.cpp
>>> @@ -244,6 +244,52 @@
>>>    MSSA.verifyMemorySSA();
>>>  }
>>>
>>> +TEST_F(MemorySSATest, SinkLoad) {
>>> +  F = Function::Create(
>>> +      FunctionType::get(B.getVoidTy(), {B.getInt8PtrTy()}, false),
>>> +      GlobalValue::ExternalLinkage, "F", &M);
>>> +  BasicBlock *Entry(BasicBlock::Create(C, "", F));
>>> +  BasicBlock *Left(BasicBlock::Create(C, "", F));
>>> +  BasicBlock *Right(BasicBlock::Create(C, "", F));
>>> +  BasicBlock *Merge(BasicBlock::Create(C, "", F));
>>> +  B.SetInsertPoint(Entry);
>>> +  B.CreateCondBr(B.getTrue(), Left, Right);
>>> +  B.SetInsertPoint(Left, Left->begin());
>>> +  Argument *PointerArg = &*F->arg_begin();
>>> +  B.SetInsertPoint(Left);
>>> +  B.CreateBr(Merge);
>>> +  B.SetInsertPoint(Right);
>>> +  B.CreateBr(Merge);
>>> +
>>> +  // Load in left block
>>> +  B.SetInsertPoint(Left, Left->begin());
>>> +  LoadInst *LoadInst1 = B.CreateLoad(PointerArg);
>>> +  // Store in merge block
>>> +  B.SetInsertPoint(Merge, Merge->begin());
>>> +  B.CreateStore(B.getInt8(16), PointerArg);
>>> +
>>> +  setupAnalyses();
>>> +  MemorySSA &MSSA = *Analyses->MSSA;
>>> +  MemorySSAUpdater Updater(&MSSA);
>>> +
>>> +  // Mimic sinking of a load:
>>> +  // - clone load
>>> +  // - insert in "exit" block
>>> +  // - insert in mssa
>>> +  // - remove from original block
>>> +
>>> +  LoadInst *LoadInstClone = cast<LoadInst>(LoadInst1->clone());
>>> +  Merge->getInstList().insert(Merge->begin(), LoadInstClone);
>>> +  MemoryAccess * NewLoadAccess =
>>> +      Updater.createMemoryAccessInBB(LoadInstClone, nullptr,
>>> +                                     LoadInstClone->getParent(),
>>> +                                     MemorySSA::Beginning);
>>> +  Updater.insertUse(cast<MemoryUse>(NewLoadAccess));
>>> +  MSSA.verifyMemorySSA();
>>> +  Updater.removeMemoryAccess(MSSA.getMemoryAccess(LoadInst1));
>>> +  MSSA.verifyMemorySSA();
>>> +}
>>> +
>>>  TEST_F(MemorySSATest, MoveAStore) {
>>>    // We create a diamond where there is a in the entry, a store on one
>>> side, and
>>>    // a load at the end.  After building MemorySSA, we test updating by
>>> moving
>>>
>>>
>>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170607/6f353091/attachment.html>