<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Oct 30, 2016 at 5:03 PM, Bryant Wong <span dir="ltr"><<a href="mailto:3.14472+reviews.llvm.org@gmail.com" target="_blank">3.14472+reviews.llvm.org@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">To give this a bit of context, this patch stems from issues that I've<br>encountered while porting MemCpyOpt to MSSA. </div></blockquote><div><br></div><div>Okay. I'm not sure i would try to port instead of just rewrite.  The whole goal of MemorySSA is to enable us to write memory optimizations in non-N^2 ways.</div><div><br></div><div>If you just replace one querying with the other querying, this is not likely to give you this result.</div><div>It may be faster (or slower).</div><div><br></div><div>MemCpyOpt currently iterates in no particular order.<br></div><div>It should be possible to do it all in a single iteration, either bottom up or top down.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Many of the transforms in this pass<br>create entirely new memsets, memcpys, and memmoves (among others). After each<br>new insertion, I expect to also have to update MSSA since 1)<br>MemorySSAWrapperPass is delcared preserved to the pass manager, and 2)<br>subsequent MCO iterations which depend on MSSA need to be able to see, analyze,<br>and possibly further transform these newly added meminsts.</div></blockquote><div><br></div><div>Can you give an example of a case where, processing in top-down order, going from defs to uses, it's not possible to do this in a single iteration?<br><br></div><div>I understand why, giving a random ordering it does it in now, and no use-def/def-use chains, it cannot do so.</div><div>I cannot, off the top of my head, think of a case it would not be possible to do in a single iteration.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br>Here's an example: MemCpyOptPass::tryMergingIntoM<wbr>emset will convert the<br>sequences of stores to %P into a memset:<br><br>  define void @foo(i64* nocapture %P, i64* %Q) {<br>  entry:<br>    %0 = bitcast i64* %P to i16*<br>    %arrayidx = getelementptr inbounds i16, i16* %0, i64 1<br>    %1 = bitcast i16* %arrayidx to i32*<br>    %arrayidx1 = getelementptr inbounds i16, i16* %0, i64 3<br>  ; 1 = MemoryDef(liveOnEntry)<br>    store i16 0, i16* %0, align 2<br>  ; 2 = MemoryDef(1)<br>    store i32 0, i32* %1, align 4<br>  ; 3 = MemoryDef(2)<br>    store i16 0, i16* %arrayidx1, align 2<br>  ; 4 = MemoryDef(3)<br>    store i64 0, i64* %Q<br>    ret void<br>  }<br><br>Specifically, a memset is first inserted right before the store to %Q:<br><br>  define void @foo(i64* nocapture %P, i64* %Q) {<br>  entry:<br>    %0 = bitcast i64* %P to i16*<br>    %arrayidx = getelementptr inbounds i16, i16* %0, i64 1<br>    %1 = bitcast i16* %arrayidx to i32*<br>    %arrayidx1 = getelementptr inbounds i16, i16* %0, i64 3<br>  ; 1 = MemoryDef(liveOnEntry)<br>    store i16 0, i16* %0, align 2<br>  ; 2 = MemoryDef(1)<br>    store i32 0, i32* %1, align 4<br>  ; 3 = MemoryDef(2)<br>    store i16 0, i16* %arrayidx1, align 2<br><br>    call void @llvm.memset.p0i8.i64(i8* %2, i8 0, i64 8, i32 2, i1 false)  ; new<br><br>  ; 4 = MemoryDef(3)<br>    store i64 0, i64* %Q<br>    ret void<br>  }<br><br>Next, a new MemoryAccess is created by passing to createMemoryAccessBefore the<br>memset instruction, `3 = MemoryDef(2)` (the new access's defining access), </div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">and<br>`4 = MemoryDef(3)` (the new access's insertion point).<br></div></blockquote><div><br></div><div>This won't work, of course. </div><div><br></div><div>However, this memset is being created to eliminate stores, no?<br>What stores are you eliminating?</div><div><br></div><div>If it it's the stores at 1, 2, and 3, then for sure, the correct defining access is *not* "3 = MemoryDef".</div><div><br>It's going to be "liveOnEntry" after you kill the stores.</div><div><br></div><div>Then, RAUW on each of the MemoryDef's works fine, and you removeAccess them.</div><div><br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br>  define void @foo(i64* nocapture %P, i64* %Q) {<br>  entry:<br>    %0 = bitcast i64* %P to i16*<br>    %arrayidx = getelementptr inbounds i16, i16* %0, i64 1<br>    %1 = bitcast i16* %arrayidx to i32*<br>    %arrayidx1 = getelementptr inbounds i16, i16* %0, i64 3<br>  ; 1 = MemoryDef(liveOnEntry)<br>    store i16 0, i16* %0, align 2<br>  ; 2 = MemoryDef(1)<br>    store i32 0, i32* %1, align 4<br>  ; 3 = MemoryDef(2)<br>    store i16 0, i16* %arrayidx1, align 2<br><br>  ; 5 = MemoryDef(3)<br>    call void @llvm.memset.p0i8.i64(i8* %2, i8 0, i64 8, i32 2, i1 false)  ; new<br><br>  ; 4 = MemoryDef(3)<br>    store i64 0, i64* %Q<br>    ret void<br>  }<br><br>This is already problematic because there's no way to selectively update the<br>post-5 MemoryDefs that point to 5 instead of 3. Calling RAUW -- specifically,<br>replacing all uses of 3 with 5 -- would result in this:<br></div></blockquote><div><br>I'm aware the existing APIs will not do what you want, because, as i said, they are specifically and documented to only  be meant for replacement.</div><div><br></div><div><br></div><div>When i originally had API's to allow for more direct manipulation, folks instead said "please build utilities to make common transforms work".</div><div><br></div><div>Thus, the existing API's make *replacement* work.</div><div><br></div><div>No amount of futzing is going to make them work well for insertion.</div><div><br></div><div>If you want on the fly updating for stores, where you aren't doing replacement, that's quite expensive.</div><div><br>MemCpyOpt is mostly doing replacement, however, and you can make them work if you actually do replacement as replacement.</div><div><br></div><div>This likely requires rethinking memcpyopt a bit.</div><div><br></div></div></div></div>