<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:SimSun;
panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
@font-face
{font-family:"\@SimSun";
panose-1:2 1 6 0 3 1 1 1 1 1;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:12.0pt;
font-family:"Times New Roman",serif;
color:black;}
span.EmailStyle20
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle21
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle24
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body bgcolor="white" lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Review
<a href="https://reviews.llvm.org/D40320">https://reviews.llvm.org/D40320</a> created.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Sam<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"><o:p> </o:p></span></p>
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> Liu, Yaxun (Sam)
<br>
<b>Sent:</b> Tuesday, November 21, 2017 2:58 PM<br>
<b>To:</b> 'Friedman, Eli' <efriedma@codeaurora.org>; llvm-commits@lists.llvm.org<br>
<b>Subject:</b> RE: [llvm] r318727 - [AMDGPU] Fix DAGTypeLegalizer::SplitInteger for shift amount type<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">That’s sounds reasonable. I will open a review. Thanks.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Sam<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"><o:p> </o:p></span></p>
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> Friedman, Eli [<a href="mailto:efriedma@codeaurora.org">mailto:efriedma@codeaurora.org</a>]
<br>
<b>Sent:</b> Tuesday, November 21, 2017 2:44 PM<br>
<b>To:</b> Liu, Yaxun (Sam) <<a href="mailto:Yaxun.Liu@amd.com">Yaxun.Liu@amd.com</a>>;
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<b>Subject:</b> Re: [llvm] r318727 - [AMDGPU] Fix DAGTypeLegalizer::SplitInteger for shift amount type<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">I would rather move the actual code to adjust the shift amount type, instead of just the assert.<br>
<br>
-Eli<br>
<br>
On 11/21/2017 9:08 AM, Liu, Yaxun (Sam) wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">How about moving the assert to
</span>LegalizeTypes.cpp, right before the change? Since that’s where regression may happen.<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Thanks.<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Sam<o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span><o:p></o:p></p>
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> Friedman, Eli [<a href="mailto:efriedma@codeaurora.org">mailto:efriedma@codeaurora.org</a>]
<br>
<b>Sent:</b> Monday, November 20, 2017 9:46 PM<br>
<b>To:</b> Liu, Yaxun (Sam) <a href="mailto:Yaxun.Liu@amd.com"><Yaxun.Liu@amd.com></a>;
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<b>Subject:</b> Re: [llvm] r318727 - [AMDGPU] Fix DAGTypeLegalizer::SplitInteger for shift amount type</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<div>
<p class="MsoNormal">On 11/20/2017 6:29 PM, Yaxun Liu via llvm-commits wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>Author: yaxunl<o:p></o:p></pre>
<pre>Date: Mon Nov 20 18:29:54 2017<o:p></o:p></pre>
<pre>New Revision: 318727<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=318727&view=rev">http://llvm.org/viewvc/llvm-project?rev=318727&view=rev</a><o:p></o:p></pre>
<pre>Log:<o:p></o:p></pre>
<pre>[AMDGPU] Fix DAGTypeLegalizer::SplitInteger for shift amount type<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>DAGTypeLegalizer::SplitInteger uses default pointer size as shift amount constant type,<o:p></o:p></pre>
<pre>which causes less performant ISA in amdgcn---amdgiz target since the default pointer<o:p></o:p></pre>
<pre>type is i64 whereas the desired shift amount type is i32.<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>This patch fixes that by using TLI.getScalarShiftAmountTy in DAGTypeLegalizer::SplitInteger.<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>The X86 change is necessary since splitting i512 requires shifting amount of 256, which<o:p></o:p></pre>
<pre>cannot be held by i8.<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>Differential Revision: <a href="https://reviews.llvm.org/D40148">https://reviews.llvm.org/D40148</a><o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>Modified:<o:p></o:p></pre>
<pre> llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp<o:p></o:p></pre>
<pre> llvm/trunk/lib/Target/X86/X86ISelLowering.h<o:p></o:p></pre>
<pre> llvm/trunk/test/CodeGen/AMDGPU/insert_vector_elt.v2i16.ll<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>Modified: llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp<o:p></o:p></pre>
<pre>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp?rev=318727&r1=318726&r2=318727&view=diff">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp?rev=318727&r1=318726&r2=318727&view=diff</a><o:p></o:p></pre>
<pre>==============================================================================<o:p></o:p></pre>
<pre>--- llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp (original)<o:p></o:p></pre>
<pre>+++ llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp Mon Nov 20 18:29:54 2017<o:p></o:p></pre>
<pre>@@ -1172,9 +1172,11 @@ void DAGTypeLegalizer::SplitInteger(SDVa<o:p></o:p></pre>
<pre> assert(LoVT.getSizeInBits() + HiVT.getSizeInBits() ==<o:p></o:p></pre>
<pre> Op.getValueSizeInBits() && "Invalid integer splitting!");<o:p></o:p></pre>
<pre> Lo = DAG.getNode(ISD::TRUNCATE, dl, LoVT, Op);<o:p></o:p></pre>
<pre>- Hi = DAG.getNode(ISD::SRL, dl, Op.getValueType(), Op,<o:p></o:p></pre>
<pre>- DAG.getConstant(LoVT.getSizeInBits(), dl,<o:p></o:p></pre>
<pre>- TLI.getPointerTy(DAG.getDataLayout())));<o:p></o:p></pre>
<pre>+ Hi =<o:p></o:p></pre>
<pre>+ DAG.getNode(ISD::SRL, dl, Op.getValueType(), Op,<o:p></o:p></pre>
<pre>+ DAG.getConstant(LoVT.getSizeInBits(), dl,<o:p></o:p></pre>
<pre>+ TLI.getScalarShiftAmountTy(<o:p></o:p></pre>
<pre>+ DAG.getDataLayout(), Op.getValueType())));<o:p></o:p></pre>
<pre> Hi = DAG.getNode(ISD::TRUNCATE, dl, HiVT, Hi);<o:p></o:p></pre>
<pre> }<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>Modified: llvm/trunk/lib/Target/X86/X86ISelLowering.h<o:p></o:p></pre>
<pre>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.h?rev=318727&r1=318726&r2=318727&view=diff">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.h?rev=318727&r1=318726&r2=318727&view=diff</a><o:p></o:p></pre>
<pre>==============================================================================<o:p></o:p></pre>
<pre>--- llvm/trunk/lib/Target/X86/X86ISelLowering.h (original)<o:p></o:p></pre>
<pre>+++ llvm/trunk/lib/Target/X86/X86ISelLowering.h Mon Nov 20 18:29:54 2017<o:p></o:p></pre>
<pre>@@ -18,6 +18,7 @@<o:p></o:p></pre>
<pre> #include "llvm/CodeGen/CallingConvLower.h"<o:p></o:p></pre>
<pre> #include "llvm/CodeGen/SelectionDAG.h"<o:p></o:p></pre>
<pre> #include "llvm/CodeGen/TargetLowering.h"<o:p></o:p></pre>
<pre>+#include "llvm/Support/MathExtras.h"<o:p></o:p></pre>
<pre> #include "llvm/Target/TargetOptions.h"<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> namespace llvm {<o:p></o:p></pre>
<pre>@@ -664,8 +665,14 @@ namespace llvm {<o:p></o:p></pre>
<pre> void markLibCallAttributes(MachineFunction *MF, unsigned CC,<o:p></o:p></pre>
<pre> ArgListTy &Args) const override;<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>- MVT getScalarShiftAmountTy(const DataLayout &, EVT) const override {<o:p></o:p></pre>
<pre>- return MVT::i8;<o:p></o:p></pre>
<pre>+ // For i512, DAGTypeLegalizer::SplitInteger needs a shift amount 256,<o:p></o:p></pre>
<pre>+ // which cannot be held by i8, therefore use i16 instead. In all the<o:p></o:p></pre>
<pre>+ // other situations i8 is sufficient.<o:p></o:p></pre>
<pre>+ MVT getScalarShiftAmountTy(const DataLayout &, EVT VT) const override {<o:p></o:p></pre>
<pre>+ MVT T = VT.getSizeInBits() >= 512 ? MVT::i16 : MVT::i8;<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal"><br>
From LangRef, the description of integer types: "Any bit width from 1 bit to 2<sup>23</sup>-1 (about 8 million) can be specified." You need at least an i32 to hold every possible shift amount. (Granted, the issue is unlikely to come up in practice.)<br>
<br>
AVR also has a getScalarShiftAmountTy() which returns i8. It would be better to add a check somewhere to the target-independent code to avoid this sort of issue rather than make each target add this check to its own getScalarShiftAmountTy().<o:p></o:p></p>
<p>-Eli<o:p></o:p></p>
<pre>-- <o:p></o:p></pre>
<pre>Employee of Qualcomm Innovation Center, Inc.<o:p></o:p></pre>
<pre>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project<o:p></o:p></pre>
</blockquote>
<p><o:p> </o:p></p>
<pre>-- <o:p></o:p></pre>
<pre>Employee of Qualcomm Innovation Center, Inc.<o:p></o:p></pre>
<pre>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project<o:p></o:p></pre>
</div>
</body>
</html>