<div dir="ltr">OK, but you still need to incorporate a test case into the patch itself as one of our regression tests.</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 28, 2015 at 12:14 PM, Mandeep Singh Grang <span dir="ltr"><<a href="mailto:mgrang@codeaurora.org" target="_blank">mgrang@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><span style="color:#1f497d">Hi,<br><br></span></p><p><u></u><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><span>1.<span style="font:7.0pt "Times New Roman"">       </span></span></span><u></u><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">The existing code has a logical error where an elsif clause has been incorrectly written whereby it skips the subsequent elseif clauses.<u></u><u></u></span></p><p><u></u><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><span>2.<span style="font:7.0pt "Times New Roman"">       </span></span></span><u></u><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">I observed that in the existing code after the SROA patch a lot of “ins” instructions whenever an intrinsic is seen.<br>This generation of “ins” in order to load a 64 bit vector does not always result in efficient code.<u></u><u></u></span></p><p><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal" style="text-indent:.25in"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">I am attaching the bitcode files for a simple testcase with and without my patch.<u></u><u></u></span></p><p class="MsoNormal" style="text-indent:.25in"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal" style="text-indent:.25in"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Thanks,<u></u><u></u></span></p><p class="MsoNormal" style="text-indent:.25in"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Mandeep<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> Chandler Carruth [<a href="mailto:chandlerc@google.com" target="_blank">mailto:chandlerc@google.com</a>] <br><b>Sent:</b> Sunday, January 25, 2015 6:57 PM<br><b>To:</b> Mandeep Singh Grang<br><b>Cc:</b> Commit Messages and Patches for LLVM<br><b>Subject:</b> Re: Fix SROA for intrinsics<u></u><u></u></span></p><div><div class="h5"><p class="MsoNormal"><u></u> <u></u></p><div><p class="MsoNormal">This looks generally like a good improvement, but it has no test cases or a clear explanation of the problem it is fixing. It needs those before it can go into the tree.<u></u><u></u></p></div><div><p class="MsoNormal"><u></u> <u></u></p><div><p class="MsoNormal">On Sun, Jan 25, 2015 at 6:48 PM, Mandeep Singh Grang <<a href="mailto:mgrang@codeaurora.org" target="_blank">mgrang@codeaurora.org</a>> wrote:<u></u><u></u></p><blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-top:5.0pt;margin-right:0in;margin-bottom:5.0pt"><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Hi,</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> </span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Can the owner for SROA please take a look into the attached patch?</span><u></u><u></u></p><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> </span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Also if you are fine with the patch could you please commit it since I do not have commit rights?</span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> </span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Thanks,</span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Mandeep</span><u></u><u></u></p></div><div><div style="border:none;border-top:solid #e1e1e1 1.0pt;padding:3.0pt 0in 0in 0in"><div><p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> Owen Anderson [<a href="mailto:resistor@mac.com" target="_blank">mailto:resistor@mac.com</a>] <br><b>Sent:</b> Wednesday, January 21, 2015 8:18 PM<br><b>To:</b> Mandeep Singh Grang<br><b>Cc:</b> llvm-commits; <a href="mailto:apazos@codeaurora.org" target="_blank">apazos@codeaurora.org</a><br><b>Subject:</b> Re: Question about commit d4748bbd497b550a4e5db246c6708fcd6de542da</span><u></u><u></u></p></div></div></div><div><p class="MsoNormal"> <u></u><u></u></p></div><div><p class="MsoNormal">Ok, patches welcome. :-)<u></u><u></u></p></div><div><div><p class="MsoNormal"> <u></u><u></u></p></div></div><div><div><p class="MsoNormal">—Owen<u></u><u></u></p></div></div><div><div><p class="MsoNormal"> <u></u><u></u></p></div><div><blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"><div><div><p class="MsoNormal">On Jan 21, 2015, at 8:00 PM, Mandeep Singh Grang <<a href="mailto:mgrang@codeaurora.org" target="_blank"><span style="color:purple">mgrang@codeaurora.org</span></a>> wrote:<u></u><u></u></p></div></div><div><p class="MsoNormal"> <u></u><u></u></p></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Yes, in the scenario where we have an intrinsic and the following condition is <b>true</b>:</span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">else if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(U->getUser()))</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> </span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">But the following condition in <b>false</b>:</span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">if (II->getIntrinsicID() != Intrinsic::lifetime_start &</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">    II->getIntrinsicID() != Intrinsic::lifetime_end)</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> </span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">In this case, all other subsequent else-if’s are skipped and the logic falls through to the “return true” and we see a lot of “ins” instructions being generated.</span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">But I think it should evaluate other else if’s since some of them may meet the condition and we may return false.</span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> </span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">We saw degradation in AArch64 cortex-a57 code due to this.</span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> </span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">We modified the code on our end to the following:</span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> </span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">IntrinsicInst *II = dyn_cast<IntrinsicInst>(U->getUser());</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> </span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><start of if else clauses></span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">…</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">else if (II && II->getIntrinsicID() != Intrinsic::lifetime_start &</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">           II->getIntrinsicID() != Intrinsic::lifetime_end)</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> </span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><other else if’s></span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> </span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Making this change get us back the lost performance.</span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> </span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> </span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Thanks,</span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Mandeep</span><u></u><u></u></p></div></div><div><div style="border:none;border-top:solid #e1e1e1 1.0pt;padding:3.0pt 0in 0in 0in"><div><div><p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> Owen Anderson [<a href="mailto:resistor@mac.com" target="_blank"><span style="color:purple">mailto:resistor@mac.com</span></a>] <br><b>Sent:</b> Wednesday, January 21, 2015 6:30 PM<br><b>To:</b> Mandeep Singh Grang<br><b>Cc:</b> llvm-commits; <a href="mailto:apazos@codeaurora.org" target="_blank"><span style="color:purple">apazos@codeaurora.org</span></a><br><b>Subject:</b> Re: Question about commit d4748bbd497b550a4e5db246c6708fcd6de542da</span><u></u><u></u></p></div></div></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><u></u><u></u></p></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Yes, it is intentional. Do you have a counter example where it results in undesirable behavior?</span><u></u><u></u></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><u></u><u></u></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">-Owen</span><u></u><u></u></p></div></div></div><div><p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"><br>On Jan 21, 2015, at 6:04 PM, Mandeep Singh Grang <<a href="mailto:mgrang@codeaurora.org" target="_blank"><span style="color:#954f72">mgrang@codeaurora.org</span></a>> wrote:</span><u></u><u></u></p></div><blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Hi Owen,</span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">In the following commit, we observed that with your else-if condition all the subsequent else-if’s are skipped.</span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Could you please confirm whether this was your intent?</span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">commit d4748bbd497b550a4e5db246c6708fcd6de542da</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Author: Owen Anderson <<a href="mailto:resistor@mac.com" target="_blank"><span style="color:#954f72">resistor@mac.com</span></a>></span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Date:   Thu Aug 7 21:07:35 2014 +0000</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">    Fix a case in SROA where lifetime intrinsics could inhibit alloca promotion.  In</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">    this case, the code path dealing with vector promotion was missing the explicit</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">    checks for lifetime intrinsics that were present on the corresponding integer</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">    promotion path.</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">+++ b/lib/Transforms/Scalar/SROA.cpp</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">@@ -1659,6 +1659,10 @@ static bool isVectorPromotionViableForSlice(</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">       return false;</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">     if (!I->isSplittable())</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">       return false; // Skip any unsplittable intrinsics.</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">+  } <b>else if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(U->getUser())) {</b></span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><b><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">+    if (II->getIntrinsicID() != Intrinsic::lifetime_start &&</span></i></b><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><b><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">+        II->getIntrinsicID() != Intrinsic::lifetime_end)</span></i></b><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><b><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">+      return false;</span></i></b><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><b><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">   } </span></i></b><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">else if (U->get()->getType()->getPointerElementType()->isStructTy()) {</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">     // Disable vector promotion when there are loads or stores of an FCA.</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><i><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">     return false;</span></i><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Thanks,</span><u></u><u></u></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Mandeep</span><u></u><u></u></p></div></div></div></blockquote></div></blockquote></div><div><p class="MsoNormal"> <u></u><u></u></p></div></div><p class="MsoNormal"><Fix_SROA_for_intrinsics.patch><u></u><u></u></p><p class="MsoNormal"> <u></u><u></u></p></div></div><p class="MsoNormal" style="margin-bottom:12.0pt"><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><u></u><u></u></p></blockquote></div><p class="MsoNormal"><u></u> <u></u></p></div></div></div></div></div></blockquote></div><br></div>