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

[Alina Sbirlea via llvm-commits]

Could you clarify what to assert for?

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


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/aa4bdc72/attachment.html>