<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">LGTM. <div><br><div><div>On Mar 20, 2013, at 1:49 PM, Michael Liao <<a href="mailto:michael.liao@intel.com">michael.liao@intel.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;">Hi Nadav<br><br>Please review the attached patch fixing cost model for AVX2 vector<br>shifts. Test cases is added to keep cost consistent with the one before<br>this patch.<br><br>Yours<br>- Michael<br><br>On Tue, 2013-03-19 at 21:21 -0700, Nadav Rotem wrote:<br><blockquote type="cite">Michael,<span class="Apple-converted-space"> </span><br><br><br>The TargetTransformInfo depends on the OpActions table for determining<br>the costs of instructions. It assumes that the cost of 'legal'<br>operation is lower than the cost of custom operations. When we mark<br>everything as custom then TTI has no way of estimating the cost of<br>operations without using the cost tables. I understand that your<br>change is needed in order to move logic from DAGCombine to the<br>Lowering phase, and generally this is a good change. Can you please<br>take a look at the cost tables and make sure that we did not miss<br>anything due to this change?<br><br><br>Thanks,<br>Nadav<br><br><br><br><br>On Mar 19, 2013, at 7:28 PM, Michael Liao <<a href="mailto:michael.liao@intel.com">michael.liao@intel.com</a>><br>wrote:<br><br><blockquote type="cite">Author: hliao<br>Date: Tue Mar 19 21:28:20 2013<br>New Revision: 177477<br><br>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=177477&view=rev">http://llvm.org/viewvc/llvm-project?rev=177477&view=rev</a><br>Log:<br>Mark all variable shifts needing customizing<br><br>- Prepare moving logic from DAG combining into DAG lowering. There's<br>no<br>functionality change.<br><br><br>Modified:<br> llvm/trunk/lib/Target/X86/X86ISelLowering.cpp<br><br>Modified: llvm/trunk/lib/Target/X86/X86ISelLowering.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=177477&r1=177476&r2=177477&view=diff">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=177477&r1=177476&r2=177477&view=diff</a><br>==============================================================================<br>--- llvm/trunk/lib/Target/X86/X86ISelLowering.cpp (original)<br>+++ llvm/trunk/lib/Target/X86/X86ISelLowering.cpp Tue Mar 19<br>21:28:20 2013<br>@@ -1053,23 +1053,16 @@ X86TargetLowering::X86TargetLowering(X86<br> setOperationAction(ISD::SRA, MVT::v8i16, Custom);<br> setOperationAction(ISD::SRA, MVT::v16i8, Custom);<br><br>- if (Subtarget->hasInt256()) {<br>- setOperationAction(ISD::SRL, MVT::v2i64, Legal);<br>- setOperationAction(ISD::SRL, MVT::v4i32, Legal);<br>-<br>- setOperationAction(ISD::SHL, MVT::v2i64, Legal);<br>- setOperationAction(ISD::SHL, MVT::v4i32, Legal);<br>+ // In the customized shift lowering, the legal cases in AVX2<br>will be<br>+ // recognized.<br>+ setOperationAction(ISD::SRL, MVT::v2i64, Custom);<br>+ setOperationAction(ISD::SRL, MVT::v4i32, Custom);<br><br>- setOperationAction(ISD::SRA, MVT::v4i32, Legal);<br>- } else {<br>- setOperationAction(ISD::SRL, MVT::v2i64, Custom);<br>- setOperationAction(ISD::SRL, MVT::v4i32, Custom);<br>+ setOperationAction(ISD::SHL, MVT::v2i64, Custom);<br>+ setOperationAction(ISD::SHL, MVT::v4i32, Custom);<br><br>- setOperationAction(ISD::SHL, MVT::v2i64, Custom);<br>- setOperationAction(ISD::SHL, MVT::v4i32, Custom);<br>+ setOperationAction(ISD::SRA, MVT::v4i32, Custom);<br><br>- setOperationAction(ISD::SRA, MVT::v4i32, Custom);<br>- }<br> setOperationAction(ISD::SDIV, MVT::v8i16, Custom);<br> setOperationAction(ISD::SDIV, MVT::v4i32, Custom);<br> }<br>@@ -1186,14 +1179,6 @@ X86TargetLowering::X86TargetLowering(X86<br><br> setOperationAction(ISD::VSELECT, MVT::v32i8, Legal);<br><br>- setOperationAction(ISD::SRL, MVT::v4i64, Legal);<br>- setOperationAction(ISD::SRL, MVT::v8i32, Legal);<br>-<br>- setOperationAction(ISD::SHL, MVT::v4i64, Legal);<br>- setOperationAction(ISD::SHL, MVT::v8i32, Legal);<br>-<br>- setOperationAction(ISD::SRA, MVT::v8i32, Legal);<br>-<br> setOperationAction(ISD::SDIV, MVT::v8i32, Custom);<br> } else {<br> setOperationAction(ISD::ADD, MVT::v4i64, Custom);<br>@@ -1210,15 +1195,17 @@ X86TargetLowering::X86TargetLowering(X86<br> setOperationAction(ISD::MUL, MVT::v8i32, Custom);<br> setOperationAction(ISD::MUL, MVT::v16i16, Custom);<br> // Don't lower v32i8 because there is no 128-bit byte mul<br>+ }<br><br>- setOperationAction(ISD::SRL, MVT::v4i64, Custom);<br>- setOperationAction(ISD::SRL, MVT::v8i32, Custom);<br>+ // In the customized shift lowering, the legal cases in AVX2<br>will be<br>+ // recognized.<br>+ setOperationAction(ISD::SRL, MVT::v4i64, Custom);<br>+ setOperationAction(ISD::SRL, MVT::v8i32, Custom);<br><br>- setOperationAction(ISD::SHL, MVT::v4i64, Custom);<br>- setOperationAction(ISD::SHL, MVT::v8i32, Custom);<br>+ setOperationAction(ISD::SHL, MVT::v4i64, Custom);<br>+ setOperationAction(ISD::SHL, MVT::v8i32, Custom);<br><br>- setOperationAction(ISD::SRA, MVT::v8i32, Custom);<br>- }<br>+ setOperationAction(ISD::SRA, MVT::v8i32, Custom);<br><br> // Custom lower several nodes for 256-bit types.<br> for (int i = MVT::FIRST_VECTOR_VALUETYPE;<br>@@ -11626,6 +11613,20 @@ SDValue X86TargetLowering::LowerShift(SD<br> if (V.getNode())<br> return V;<br><br>+ // AVX2 has VPSLLV/VPSRAV/VPSRLV.<br>+ if (Subtarget->hasInt256()) {<br>+ if (Op.getOpcode() == ISD::SRL &&<br>+ (VT == MVT::v2i64 || VT == MVT::v4i32 ||<br>+ VT == MVT::v4i64 || VT == MVT::v8i32))<br>+ return Op;<br>+ if (Op.getOpcode() == ISD::SHL &&<br>+ (VT == MVT::v2i64 || VT == MVT::v4i32 ||<br>+ VT == MVT::v4i64 || VT == MVT::v8i32))<br>+ return Op;<br>+ if (Op.getOpcode() == ISD::SRA && (VT == MVT::v4i32 || VT ==<br>MVT::v8i32))<br>+ return Op;<br>+ }<br>+<br> // Lower SHL with variable shift amount.<br> if (VT == MVT::v4i32 && Op->getOpcode() == ISD::SHL) {<br> Op = DAG.getNode(ISD::SHL, dl, VT, Amt, DAG.getConstant(23,<br>VT));<br><br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote><br><br></blockquote><br><span><0001-Correct-cost-model-for-vector-shift-on-AVX2.patch></span></div></blockquote></div><br></div></body></html>