<div dir="ltr">Okay. I'll clean it up and add to the unittests and submit it.<div> </div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 29, 2016 at 8:46 PM, Sebastian Pop <span dir="ltr"><<a href="mailto:sebpop@gmail.com" target="_blank">sebpop@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Yes, your patch works without my last changes: i.e., keeping the<br>
natural way of search/replace an SSA name.<br>
<div class="HOEnZb"><div class="h5"><br>
On Fri, Jul 29, 2016 at 10:35 PM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:<br>
> FWIW: The patch i sent should fix the issue of being able to create them.<br>
> The code right now is clearly wrong, it assumes but does not check that<br>
> insertion never fails, etc.<br>
><br>
><br>
> On Fri, Jul 29, 2016 at 8:30 PM, Sebastian Pop <<a href="mailto:sebpop@gmail.com">sebpop@gmail.com</a>> wrote:<br>
>><br>
>> I think I have a patch that works around the limitation of only one<br>
>> MSSA per instruction:<br>
>> instead of moving the instruction that we hoist, we clone it, such<br>
>> that it can have its own separate MSSA annotation.<br>
>><br>
>><br>
>> On Fri, Jul 29, 2016 at 10:23 PM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>><br>
>> wrote:<br>
>> > Okay, so ...<br>
>> ><br>
>> > you have to create before you can remove.<br>
>> ><br>
>> > remove is going to destroy the memory, and not much we can do about<br>
>> > that.<br>
>> > (We could split remove into remove and erase, but ... ugly)<br>
>> ><br>
>> > If you create first, right now, it will do the wrong thing (the<br>
>> > annotation<br>
>> > will never end up on the instruction, unless the instruction is new)<br>
>> > Please try the attached, which should make it stop doing the wrong thing<br>
>> > there. If that works, i'll add unit tests and commit it.<br>
>> ><br>
>> > verifyMemorySSA will catch if you mess it up (verifyOrdering checks that<br>
>> > the<br>
>> > accesslist and the lookups have the same number of things).<br>
>> ><br>
>> ><br>
>> > On Fri, Jul 29, 2016 at 7:36 PM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>><br>
>> > wrote:<br>
>> >><br>
>> >> So, I think remove from lookups should check and only remove from the<br>
>> >> lookup if the lookups point to that access<br>
>> >><br>
>> >><br>
>> >> On Fri, Jul 29, 2016, 7:23 PM Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>><br>
>> >> wrote:<br>
>> >>><br>
>> >>> I have to stare at this.<br>
>> >>><br>
>> >>> On Fri, Jul 29, 2016, 7:10 PM Sebastian Pop <<a href="mailto:sebpop@gmail.com">sebpop@gmail.com</a>> wrote:<br>
>> >>>><br>
>> >>>> sebpop updated this revision to Diff 66212.<br>
>> >>>> sebpop added a comment.<br>
>> >>>><br>
>> >>>> Updated the patch as I first wrote it: I think this is the natural<br>
>> >>>> way<br>
>> >>>> to update an SSA variable.<br>
>> >>>><br>
>> >>>> This fails because when invoking:<br>
>> >>>> + MSSA->removeMemoryAccess(OldMemAcc);<br>
>> >>>><br>
>> >>>> we go from:<br>
>> >>>> ; MemoryUse(2)<br>
>> >>>><br>
>> >>>> %2 = load i32, i32* %v.addr, align 4<br>
>> >>>><br>
>> >>>> to no annotation at all:<br>
>> >>>><br>
>> >>>> %2 = load i32, i32* %v.addr, align 4<br>
>> >>>><br>
>> >>>> This is because removeMemoryAccess calls removeFromLookups which<br>
>> >>>> does:<br>
>> >>>><br>
>> >>>> Value *MemoryInst;<br>
>> >>>> if (MemoryUseOrDef *MUD = dyn_cast<MemoryUseOrDef>(MA)) {<br>
>> >>>> MemoryInst = MUD->getMemoryInst();<br>
>> >>>> } else {<br>
>> >>>> MemoryInst = MA->getBlock();<br>
>> >>>> }<br>
>> >>>> ValueToMemoryAccess.erase(MemoryInst);<br>
>> >>>><br>
>> >>>> and erase() removes both the new and old MemoryAccess'es leaving the<br>
>> >>>> instruction without MSSA annotation.<br>
>> >>><br>
>> >>><br>
>> >>> Yes, only one memory access can exist for an instruction for real.<br>
>> >>> You<br>
>> >>> can't create two at once and get the right result.<br>
>> >>><br>
>> >>>><br>
>> >>>> For loads this can be fixed by first removing the OldMemoryAccess and<br>
>> >>>> then creating the NewMemoryAccess.<br>
>> >>>> This works for loads because there are no users of a MemoryUse.<br>
>> >>>> However for a store, we need to keep both the old and new<br>
>> >>>> MemoryAccess'es at the same time, to be able to rename the uses of<br>
>> >>>> old with<br>
>> >>>> new.<br>
>> >>><br>
>> >>><br>
>> >>> Yes, most of the hoisters are cloning, so do not have this issue.<br>
>> >>><br>
>> >>>><br>
>> >>>> I think the solution would be for<br>
>> >>>> "MSSA->removeMemoryAccess(OldMemAcc);"<br>
>> >>>> to only invalidate and remove "OldMemAcc" without touching<br>
>> >>>> "NewMemAcc" that<br>
>> >>>> should remain attached to the instruction.<br>
>> >>><br>
>> >>><br>
>> >>> We can't make it work with two memoryacceses for the same instruction<br>
>> >>> at<br>
>> >>> once.<br>
>> >>><br>
>> >>> We can probably implement either a createReplacement (that takes over<br>
>> >>> the<br>
>> >>> uses of the old one) or a moveBefore/moveAfter.<br>
>> >>>><br>
>> >>>><br>
>> >>>><br>
>> >>>> <a href="https://reviews.llvm.org/D22966" rel="noreferrer" target="_blank">https://reviews.llvm.org/D22966</a><br>
>> >>>><br>
>> >>>> Files:<br>
>> >>>> lib/Transforms/Scalar/GVNHoist.cpp<br>
>> >>>><br>
>> >>>> Index: lib/Transforms/Scalar/GVNHoist.cpp<br>
>> >>>> ===================================================================<br>
>> >>>> --- lib/Transforms/Scalar/GVNHoist.cpp<br>
>> >>>> +++ lib/Transforms/Scalar/GVNHoist.cpp<br>
>> >>>> @@ -193,30 +193,27 @@<br>
>> >>>> VN.setAliasAnalysis(AA);<br>
>> >>>> VN.setMemDep(MD);<br>
>> >>>> bool Res = false;<br>
>> >>>> + MemorySSA M(F, AA, DT);<br>
>> >>>> + MSSA = &M;<br>
>> >>>><br>
>> >>>> // FIXME: use lazy evaluation of VN to avoid the fix-point<br>
>> >>>> computation.<br>
>> >>>> while (1) {<br>
>> >>>> - // FIXME: only compute MemorySSA once. We need to update the<br>
>> >>>> analysis in<br>
>> >>>> - // the same time as transforming the code.<br>
>> >>>> - MemorySSA M(F, AA, DT);<br>
>> >>>> - MSSA = &M;<br>
>> >>>> -<br>
>> >>>> // Perform DFS Numbering of instructions.<br>
>> >>>> unsigned I = 0;<br>
>> >>>> for (const BasicBlock *BB : depth_first(&F.getEntryBlock()))<br>
>> >>>> for (auto &Inst: *BB)<br>
>> >>>> DFSNumber.insert({&Inst, ++I});<br>
>> >>>><br>
>> >>>> auto HoistStat = hoistExpressions(F);<br>
>> >>>> - if (HoistStat.first + HoistStat.second == 0) {<br>
>> >>>> + if (HoistStat.first + HoistStat.second == 0)<br>
>> >>>> return Res;<br>
>> >>>> - }<br>
>> >>>> - if (HoistStat.second > 0) {<br>
>> >>>> +<br>
>> >>>> + if (HoistStat.second > 0)<br>
>> >>>> // To address a limitation of the current GVN, we need to<br>
>> >>>> rerun<br>
>> >>>> the<br>
>> >>>> - // hoisting after we hoisted loads in order to be able to<br>
>> >>>> hoist<br>
>> >>>> all<br>
>> >>>> - // scalars dependent on the hoisted loads. Same for stores.<br>
>> >>>> + // hoisting after we hoisted loads or stores in order to be<br>
>> >>>> able to<br>
>> >>>> + // hoist all scalars dependent on the hoisted ld/st.<br>
>> >>>> VN.clear();<br>
>> >>>> - }<br>
>> >>>> +<br>
>> >>>> Res = true;<br>
>> >>>><br>
>> >>>> // DFS numbers change when instructions are hoisted: clear and<br>
>> >>>> recompute.<br>
>> >>>> @@ -725,6 +722,8 @@<br>
>> >>>> if (!Repl || firstInBB(I, Repl))<br>
>> >>>> Repl = I;<br>
>> >>>><br>
>> >>>> + MemoryAccess *NewMemAcc = nullptr;<br>
>> >>>> +<br>
>> >>>> if (Repl) {<br>
>> >>>> // Repl is already in HoistPt: it remains in place.<br>
>> >>>> assert(allOperandsAvailable(Repl, HoistPt) &&<br>
>> >>>> @@ -742,7 +741,23 @@<br>
>> >>>> !makeGepOperandsAvailable(Repl, HoistPt,<br>
>> >>>> InstructionsToHoist))<br>
>> >>>> continue;<br>
>> >>>><br>
>> >>>> + // Before moving the instruction, get its MSSA access.<br>
>> >>>> + MemoryUseOrDef *OldMemAcc = nullptr;<br>
>> >>>> + if (MemoryAccess *MA = MSSA->getMemoryAccess(Repl))<br>
>> >>>> + OldMemAcc = dyn_cast<MemoryUseOrDef>(MA);<br>
>> >>>> +<br>
>> >>>> + // Move the instruction at the end of HoistPt.<br>
>> >>>> Repl->moveBefore(HoistPt->getTerminator());<br>
>> >>>> +<br>
>> >>>> + if (OldMemAcc) {<br>
>> >>>> + // The definition of this ld/st will not change: ld/st<br>
>> >>>> hoisting is<br>
>> >>>> + // legal when the ld/st is not moved past its current<br>
>> >>>> definition.<br>
>> >>>> + MemoryAccess *Def = OldMemAcc->getDefiningAccess();<br>
>> >>>> + NewMemAcc = MSSA->createMemoryAccessInBB(Repl, Def,<br>
>> >>>> HoistPt,<br>
>> >>>> + MemorySSA::End);<br>
>> >>>> + OldMemAcc->replaceAllUsesWith(NewMemAcc);<br>
>> >>>> + MSSA->removeMemoryAccess(OldMemAcc);<br>
>> >>>> + }<br>
>> >>>> }<br>
>> >>>><br>
>> >>>> if (isa<LoadInst>(Repl))<br>
>> >>>> @@ -775,6 +790,14 @@<br>
>> >>>> } else if (isa<CallInst>(Repl)) {<br>
>> >>>> ++NumCallsRemoved;<br>
>> >>>> }<br>
>> >>>> +<br>
>> >>>> + if (NewMemAcc) {<br>
>> >>>> + // Update the uses of the old MSSA access with<br>
>> >>>> NewMemAcc.<br>
>> >>>> + MemoryAccess *OldMA = MSSA->getMemoryAccess(I);<br>
>> >>>> + OldMA->replaceAllUsesWith(NewMemAcc);<br>
>> >>>> + MSSA->removeMemoryAccess(OldMA);<br>
>> >>>> + }<br>
>> >>>> +<br>
>> >>>> Repl->intersectOptionalDataWith(I);<br>
>> >>>> combineKnownMetadata(Repl, I);<br>
>> >>>> I->replaceAllUsesWith(Repl);<br>
>> >>>><br>
>> >>>><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div>