Hi David,<br><br>I had basically aimed to minimize the changes, to address one problem. I agree with you, and think there are potential problems in this area...zext vs. sext, and SIGN/ZERO_EXTEND vs. [SZ]EXTLOAD. Without the long sign/zero extension change, we'll still trigger the outer assertion...but just writing that makes me feel that we might as well put that logic in as well. Since that is essentially your patch, want to have another go? Or, would you like me to fold it into mine? I'll try and be pleasantly persistent to get either of them in :).<br>
<br>Pete<br><br><br><div class="gmail_quote">On Tue, Nov 13, 2012 at 1:33 PM, David Peixotto <span dir="ltr"><<a href="mailto:dpeixott@codeaurora.org" target="_blank">dpeixott@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 link="blue" vlink="purple" lang="EN-US"><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Hi Pete,<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"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Thanks for updating your patch. <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"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">I don’t think I was able to trigger the long sign/zero extension with a simple test case either, although I don’t know of a reason why it would not be possible to trigger that path. If we don’t want to put the full logic in to support the possibility how about we add a comment and an assertion to verify that the underlying node is a 64-bit vector. That way if our assumption is wrong we can quickly identify and fix the issue.<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"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">-David<u></u><u></u></span></p>
<div class="im"><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"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">-- </span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation<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"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
</div><div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"><div><div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0in 0in 0in"><p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Pete Couperus [mailto:<a href="mailto:pjcoup@gmail.com" target="_blank">pjcoup@gmail.com</a>] <br>
<b>Sent:</b> Monday, November 12, 2012 10:02 PM<br><b>To:</b> David Peixotto<br><b>Cc:</b> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><b>Subject:</b> Re: [llvm-commits] [PATCH] ARM: Allow long extensions to combine into vmull<u></u><u></u></span></p>
</div></div><div><div class="h5"><p class="MsoNormal"><u></u> <u></u></p><p class="MsoNormal" style="margin-bottom:12.0pt">Hi David,<br><br>Thanks for taking a look.<br>My apologies, I hadn't seen your patch when I'd written this up.<br>
And, you're right, I neglected the v2i8 case.<br>I originally had logic in for handling long sign/zero extension as well, but I couldn't trigger that with any of the basic test cases I thought of.<br>Curiously, the unsigned/long zero extension cases work in 3.1, but the codegen for unsigned v2i16->v2i64 followed by mul is substantially worse than for the signed case.<br>
All the test cases that you sent pass with this new patch (allowing the v2i8->v2i64 extension). <br>Perhaps someone else wants to take a look :).<br><br>Pete<br><br><u></u><u></u></p><div><p class="MsoNormal">On Mon, Nov 12, 2012 at 10:00 AM, David Peixotto <<a href="mailto:dpeixott@codeaurora.org" target="_blank">dpeixott@codeaurora.org</a>> wrote:<u></u><u></u></p>
<div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Hi Pete,</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">I sent a patch for this a while back, but never got any feedback and failed to push it all the way through. I’ve attached my patch for reference.</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">It looks like the patches are fairly similar in their intent. I have a few more test cases, so please include those in your patch as well. It would also be good to include some of the comments, particularly the SkipExtension comment that explains the motivation for that function.</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">The main differences between the patches that I see are:</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><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">1)</span><span style="font-size:7.0pt;color:#1f497d"> </span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Handling a sign extend that is not large enough</span><u></u><u></u></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">2)</span><span style="font-size:7.0pt;color:#1f497d"> </span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Handling v2i8 vector size</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">I’ll expand on these points below.</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">Index: lib/Target/ARM/ARMISelLowering.cpp</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">--- lib/Target/ARM/ARMISelLowering.cpp (revision 167684)</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+++ lib/Target/ARM/ARMISelLowering.cpp (working copy)</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">@@ -4922,11 +4922,30 @@</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">static SDValue SkipExtension(SDNode *N, SelectionDAG &DAG) {</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> if (N->getOpcode() == ISD::SIGN_EXTEND || N->getOpcode() == ISD::ZERO_EXTEND)</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> return N->getOperand(0);</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">What it N is a small vector here? Don’t we need to make sure that we have a sign/zero extend that will extend the size to 64-bit vector?</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">- if (LoadSDNode *LD = dyn_cast<LoadSDNode>(N))</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">- return DAG.getLoad(LD->getMemoryVT(), N->getDebugLoc(), LD->getChain(),</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">- LD->getBasePtr(), LD->getPointerInfo(), LD->isVolatile(),</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">- LD->isNonTemporal(), LD->isInvariant(),</span><u></u><u></u></p><p class="MsoNormal">
<span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">- LD->getAlignment());</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">+ if (LoadSDNode *LD = dyn_cast<LoadSDNode>(N)) {</span><u></u><u></u></p><p class="MsoNormal">
<span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ SDValue BareLD = DAG.getLoad(LD->getMemoryVT(), N->getDebugLoc(),</span><u></u><u></u></p><p class="MsoNormal">
<span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ LD->getChain(), LD->getBasePtr(),</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ LD->getPointerInfo(), LD->isVolatile(),</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ LD->isNonTemporal(), LD->isInvariant(),</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ LD->getAlignment());</span><u></u><u></u></p><p class="MsoNormal">
<span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ if (BareLD.getValueType().is64BitVector())</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ return BareLD;</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">+ // This is a long extension, v4i8 -> v4i32 or v2i16 -> v2i64</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ MVT ExtVT;</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ if (LD->getMemoryVT() == MVT::v4i8)</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ ExtVT = MVT::v4i16;</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ else if (LD->getMemoryVT() == MVT::v2i16)</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ ExtVT = MVT::v2i32;</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">What about MVT::v2i8. We can handle that case similar to v2i16 by extending the size to v2i32.</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">+</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ if (LD->getExtensionType() == ISD::SEXTLOAD)</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ return DAG.getNode(ISD::SIGN_EXTEND, BareLD.getDebugLoc(), ExtVT, BareLD);</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ else if (LD->getExtensionType() == ISD::ZEXTLOAD)</span><u></u><u></u></p><p class="MsoNormal">
<span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">+ return DAG.getNode(ISD::ZERO_EXTEND, BareLD.getDebugLoc(), ExtVT, BareLD);</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">+ llvm_unreachable("Unexpected extended load type for SkipExtension.");</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"> // Otherwise, the value must be a BUILD_VECTOR. For v2i64, it will</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> // have been legalized as a BITCAST from v4i32.</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> if (N->getOpcode() == ISD::BITCAST) {</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">-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation</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"> </span><u></u><u></u></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"><div><div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0in 0in 0in"><p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> <a href="mailto:llvm-commits-bounces@cs.uiuc.edu" target="_blank">llvm-commits-bounces@cs.uiuc.edu</a> [mailto:<a href="mailto:llvm-commits-bounces@cs.uiuc.edu" target="_blank">llvm-commits-bounces@cs.uiuc.edu</a>] <b>On Behalf Of </b>Pete Couperus<br>
<b>Sent:</b> Saturday, November 10, 2012 11:48 PM<br><b>To:</b> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><b>Subject:</b> [llvm-commits] [PATCH] ARM: Allow long extensions to combine into vmull</span><u></u><u></u></p>
</div></div><div><div><p class="MsoNormal"> <u></u><u></u></p><p class="MsoNormal" style="margin-bottom:12.0pt">Hello,<br><br>The ARM backend currently crashes when trying to combine:<br> %tmp1 = load <4 x i8>* %A<br>
%tmp2 = load <4 x i8>* %B<br> %tmp3 = sext <4 x i8> %tmp1 to <4 x i32><br> %tmp4 = sext <4 x i8> %tmp2 to <4 x i32><br> %tmp5 = mul <4 x i32> %tmp3, %tmp4<br>
<br>into a vmull. This was reported as PR 12281.<br>This patch fixes this problem.<br>Please review!<br>Thanks,<br><br>Pete<u></u><u></u></p></div></div></div></div></div></div><p class="MsoNormal"><u></u> <u></u></p></div>
</div></div></div></div></blockquote></div><br>