[PATCH] D22966: GVN-hoist: compute MSSA once per function (PR28670)

[Daniel Berlin via llvm-commits]

So, I think remove from lookups should check and only remove from the
lookup if the lookups point to that access

On Fri, Jul 29, 2016, 7:23 PM Daniel Berlin <dberlin at dberlin.org> wrote:

> I have to stare at this.
>
> On Fri, Jul 29, 2016, 7:10 PM Sebastian Pop <sebpop at gmail.com> wrote:
>
>> sebpop updated this revision to Diff 66212.
>> sebpop added a comment.
>>
>> Updated the patch as I first wrote it: I think this is the natural way to
>> update an SSA variable.
>>
>> This fails because when invoking:
>> +          MSSA->removeMemoryAccess(OldMemAcc);
>>
>> we go from:
>> ; MemoryUse(2)
>>
>>   %2 = load i32, i32* %v.addr, align 4
>>
>> to no annotation at all:
>>
>>   %2 = load i32, i32* %v.addr, align 4
>>
>> This is because removeMemoryAccess calls removeFromLookups which does:
>>
>>   Value *MemoryInst;
>>   if (MemoryUseOrDef *MUD = dyn_cast<MemoryUseOrDef>(MA)) {
>>     MemoryInst = MUD->getMemoryInst();
>>   } else {
>>     MemoryInst = MA->getBlock();
>>   }
>>   ValueToMemoryAccess.erase(MemoryInst);
>>
>> and erase() removes both the new and old MemoryAccess'es leaving the
>> instruction without MSSA annotation.
>>
>
> Yes, only one memory access can exist for an instruction for real.  You
> can't create two at once and get the right result.
>
>
>> For loads this can be fixed by first removing the OldMemoryAccess and
>> then creating the NewMemoryAccess.
>> This works for loads because there are no users of a MemoryUse.
>> However for a store, we need to keep both the old and new MemoryAccess'es
>> at the same time, to be able to rename the uses of old with new.
>>
>
> Yes, most of the hoisters are cloning, so do not have this issue.
>
>
>> I think the solution would be for "MSSA->removeMemoryAccess(OldMemAcc);"
>> to only invalidate and remove "OldMemAcc" without touching "NewMemAcc" that
>> should remain attached to the instruction.
>>
>
> We can't make it work with two memoryacceses for the same instruction at
> once.
>
> We can probably implement either a createReplacement (that takes over the
> uses of the old one) or a moveBefore/moveAfter.
>
>>
>>
>> https://reviews.llvm.org/D22966
>>
>> Files:
>>   lib/Transforms/Scalar/GVNHoist.cpp
>>
>> Index: lib/Transforms/Scalar/GVNHoist.cpp
>> ===================================================================
>> --- lib/Transforms/Scalar/GVNHoist.cpp
>> +++ lib/Transforms/Scalar/GVNHoist.cpp
>> @@ -193,30 +193,27 @@
>>      VN.setAliasAnalysis(AA);
>>      VN.setMemDep(MD);
>>      bool Res = false;
>> +    MemorySSA M(F, AA, DT);
>> +    MSSA = &M;
>>
>>      // FIXME: use lazy evaluation of VN to avoid the fix-point
>> computation.
>>      while (1) {
>> -      // FIXME: only compute MemorySSA once. We need to update the
>> analysis in
>> -      // the same time as transforming the code.
>> -      MemorySSA M(F, AA, DT);
>> -      MSSA = &M;
>> -
>>        // Perform DFS Numbering of instructions.
>>        unsigned I = 0;
>>        for (const BasicBlock *BB : depth_first(&F.getEntryBlock()))
>>          for (auto &Inst: *BB)
>>            DFSNumber.insert({&Inst, ++I});
>>
>>        auto HoistStat = hoistExpressions(F);
>> -      if (HoistStat.first + HoistStat.second == 0) {
>> +      if (HoistStat.first + HoistStat.second == 0)
>>          return Res;
>> -      }
>> -      if (HoistStat.second > 0) {
>> +
>> +      if (HoistStat.second > 0)
>>          // To address a limitation of the current GVN, we need to rerun
>> the
>> -        // hoisting after we hoisted loads in order to be able to hoist
>> all
>> -        // scalars dependent on the hoisted loads. Same for stores.
>> +        // hoisting after we hoisted loads or stores in order to be able
>> to
>> +        // hoist all scalars dependent on the hoisted ld/st.
>>          VN.clear();
>> -      }
>> +
>>        Res = true;
>>
>>        // DFS numbers change when instructions are hoisted: clear and
>> recompute.
>> @@ -725,6 +722,8 @@
>>            if (!Repl || firstInBB(I, Repl))
>>              Repl = I;
>>
>> +      MemoryAccess *NewMemAcc = nullptr;
>> +
>>        if (Repl) {
>>          // Repl is already in HoistPt: it remains in place.
>>          assert(allOperandsAvailable(Repl, HoistPt) &&
>> @@ -742,7 +741,23 @@
>>              !makeGepOperandsAvailable(Repl, HoistPt,
>> InstructionsToHoist))
>>            continue;
>>
>> +        // Before moving the instruction, get its MSSA access.
>> +        MemoryUseOrDef *OldMemAcc = nullptr;
>> +        if (MemoryAccess *MA = MSSA->getMemoryAccess(Repl))
>> +          OldMemAcc = dyn_cast<MemoryUseOrDef>(MA);
>> +
>> +        // Move the instruction at the end of HoistPt.
>>          Repl->moveBefore(HoistPt->getTerminator());
>> +
>> +        if (OldMemAcc) {
>> +          // The definition of this ld/st will not change: ld/st
>> hoisting is
>> +          // legal when the ld/st is not moved past its current
>> definition.
>> +          MemoryAccess *Def = OldMemAcc->getDefiningAccess();
>> +          NewMemAcc = MSSA->createMemoryAccessInBB(Repl, Def, HoistPt,
>> +                                                   MemorySSA::End);
>> +          OldMemAcc->replaceAllUsesWith(NewMemAcc);
>> +          MSSA->removeMemoryAccess(OldMemAcc);
>> +        }
>>        }
>>
>>        if (isa<LoadInst>(Repl))
>> @@ -775,6 +790,14 @@
>>            } else if (isa<CallInst>(Repl)) {
>>              ++NumCallsRemoved;
>>            }
>> +
>> +          if (NewMemAcc) {
>> +            // Update the uses of the old MSSA access with NewMemAcc.
>> +            MemoryAccess *OldMA = MSSA->getMemoryAccess(I);
>> +            OldMA->replaceAllUsesWith(NewMemAcc);
>> +            MSSA->removeMemoryAccess(OldMA);
>> +          }
>> +
>>            Repl->intersectOptionalDataWith(I);
>>            combineKnownMetadata(Repl, I);
>>            I->replaceAllUsesWith(Repl);
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160730/194d90c1/attachment.html>