<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Nov 20, 2015 at 11:57 AM Quentin Colombet via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><blockquote type="cite"><div>On Nov 20, 2015, at 9:55 AM, Arnaud A. de Grandmaison <<a href="mailto:arnaud.degrandmaison@arm.com" target="_blank">arnaud.degrandmaison@arm.com</a>> wrote:</div><br><div><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><blockquote type="cite" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">-----Original Message-----<br>From: <a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a> [<a href="mailto:qcolombet@apple.com" target="_blank">mailto:qcolombet@apple.com</a>]<br>Sent: 20 November 2015 18:07<br>To: Arnaud De Grandmaison<br>Cc: <a href="mailto:haicheng@codeaurora.org" target="_blank">haicheng@codeaurora.org</a>; <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>Subject: Re: [llvm-dev] [AArch64] bug in shrink-wrapping<br><br>Hi Arnaud,<br><br>Thanks for following up with that and sorry for the breakage.<br><br>Couple of comments:<br>  MachineLoopInfo *MLI;<br>+  RegScavenger *RS;<br><br>Would it make sense to use a unique_ptr here?<br>That should eliminate the need of having explicit deletes.<br></blockquote><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">Using unique_ptr was my first attempt at fixing the memory leak :)<span> </span></span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">I do not think you can, unless there is a way to "reset" (or create for the</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">first time) the register scavenger for each function which requires it.</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">The logical place where to put the unique_ptr would be in the</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">runOnMachineFunction (and not as a class member) because this method has</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">multiple exit paths, but the scavenger has to be passed around several calls</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">then.</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"></div></blockquote><div><br></div></div></div><div style="word-wrap:break-word"><div>Looks like you have a better hand on this than me :).<br><blockquote type="cite"><div></div></blockquote></div></div><div style="word-wrap:break-word"><div><blockquote type="cite"><div><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><blockquote type="cite" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br>+; RUN: llc -mtriple=aarch64-linux-gnu -o - %s<br><br>Add -enable-shrink-wrap=true and a second RUN line with -enable-shrink-<br>wrap=false.<br>Then add check lines for both to ensure shrink-wrapping is doing the right<br>thing.<br></blockquote><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">I was checking here for the crash only, so not having a crash is a</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">successfully passing test. I should maybe improve the comment.<span> </span></span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">If I understand you well, you want to additionally check shrink-wrapping is</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">doing the right thing as it seems there was a coverage hole there.</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">I would suggest that this is added as a follow-up patch, as this is for now</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">breaking internal bots and it may take a bit of time for me to understand</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"></div></blockquote></div></div><div style="word-wrap:break-word"><div><blockquote type="cite"><div><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">what are the actual expectations from shrink-wrap.</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"></div></blockquote><div><br></div>Fair enough, please proceed!</div></div></blockquote><div><br></div><div>FWIW I'm running into this as well :)</div><div><br></div><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div>Thanks,</div><div>Q.</div></div><div style="word-wrap:break-word"><div><br><blockquote type="cite"><div><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><blockquote type="cite" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br>+  %0 = load i32, i32* @g1, align 4<br>Please use opt -instnamer to get rid of the numbered variables. Those are<br></blockquote><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">a</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><blockquote type="cite" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">pain when updating the tests :).<br></blockquote><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">Sure, will do.</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><blockquote type="cite" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br>Other than that LGTM!<br><br>Cheers,<br>-Quentin<br><br><blockquote type="cite">On Nov 20, 2015, at 6:31 AM, Arnaud A. de Grandmaison<br></blockquote><<a href="mailto:arnaud.degrandmaison@arm.com" target="_blank">arnaud.degrandmaison@arm.com</a>> wrote:<br><blockquote type="cite"><br>+CC llvm-dev<br><br><blockquote type="cite">-----Original Message-----<br>From: Arnaud A. de Grandmaison<br></blockquote></blockquote>[<a href="mailto:arnaud.degrandmaison@arm.com" target="_blank">mailto:arnaud.degrandmaison@arm.com</a>]<br><blockquote type="cite"><blockquote type="cite">Sent: 20 November 2015 15:28<br>To: '<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>'<br>Cc: '<a href="mailto:haicheng@codeaurora.org" target="_blank">haicheng@codeaurora.org</a>'<br>Subject: RE: [llvm-dev] [AArch64] bug in shrink-wrapping<br><br>Now with memory leak addressed.<br><br>Cheers,<br>Arnaud<br><br><blockquote type="cite">-----Original Message-----<br>From: Arnaud A. de Grandmaison<br></blockquote>[<a href="mailto:arnaud.degrandmaison@arm.com" target="_blank">mailto:arnaud.degrandmaison@arm.com</a>]<br><blockquote type="cite">Sent: 20 November 2015 14:42<br>To: '<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>'<br>Cc: '<a href="mailto:haicheng@codeaurora.org" target="_blank">haicheng@codeaurora.org</a>'<br>Subject: RE: [llvm-dev] [AArch64] bug in shrink-wrapping<br><br>There is a memory leak in my previous patch, let me fix it.<br><br>Cheers,<br>Arnaud<br><br><blockquote type="cite">-----Original Message-----<br>From: Arnaud A. de Grandmaison<br></blockquote>[<a href="mailto:arnaud.degrandmaison@arm.com" target="_blank">mailto:arnaud.degrandmaison@arm.com</a>]<br><blockquote type="cite">Sent: 20 November 2015 12:49<br>To: <a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a><br>Cc: '<a href="mailto:haicheng@codeaurora.org" target="_blank">haicheng@codeaurora.org</a>'<br>Subject: RE: [llvm-dev] [AArch64] bug in shrink-wrapping<br><br>Hi Quentin,<br><br>I believe the attached patch fixes the issue. Can you review it ?<br><br>Cheers,<br>Arnaud<br><br><blockquote type="cite">-----Original Message-----<br>From: llvm-dev [<a href="mailto:llvm-dev-bounces@lists.llvm.org" target="_blank">mailto:llvm-dev-bounces@lists.llvm.org</a>] On Behalf<br>Of via llvm-dev<br>Sent: 20 November 2015 05:37<br>To: <a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a><br>Cc: <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>Subject: [llvm-dev] [AArch64] bug in shrink-wrapping<br><br>Hi Quentin,<br><br>After shrink-wrapping was enabled as default on AArch64, llc has a<br>seg fault when compiling the attached .ll file on AArch64.<br><br>My command is<br><br>llc -mcpu=cortex-a57 bug.ll<br><br>Best,<br><br>Haicheng<br></blockquote></blockquote></blockquote></blockquote><0001-ShrinkWrap-Teach-ShrinkWrap-to-handle-targets-requir.patch></blockquote></blockquote></div></blockquote></div></div>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div>