<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Almost there! You forgot the LoopVectorizer, which should be easy to fix. </div><div><br></div><div>Thanks,</div><div>Nadav</div><div><br></div><br><div><div>On Feb 12, 2014, at 12:04 PM, Andrea Di Biagio <<a href="mailto:andrea.dibiagio@gmail.com">andrea.dibiagio@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Hi Nadav,<br><br>This new patch should fix the Vectorizer as well.<br><br>Differences are:<br>- method 'getEntryCost' in SLPVectorizer is now able to correctly<br>identify uniform and non-uniform constant values.<br>- method getInstrCost in the BBVectorize now accepts two extra<br>operands of type OperandValueKind.<br>- method areInstsCompatible in BBVectorize is now able to correctly<br>compute the cost of a merged shift instruction based on the knowledge<br>of the operand value kinds.<br> Example 1.<br> given the following pair:<br> %shl1 = shl A, 1<br> %shl2 = shl B, 1<br><br> The operand value kind for the second operand of the merged shift<br>will be equal to OK_UniformConstantValue<br><br> Example 2.<br> given the following pair:<br> %shl1 = shl A, 1<br> %shl2 = shl B, 2<br><br> The operand value kind for the second operand of the merged shift<br>will be equal to OK_NonUniformConstantValue.<br><br><br>Please let me know if ok to submit.<br><br>Thanks :-)<br>Andrea<br><br>P.s.: on a slightly different topic.<br>While testing the SLP vectorizer, I noticed that method<br>'vectorizeStoreChain' conservatively limits the length of a<br>vectorizable chain of instructions based on the value of the global<br>variable 'MinVecRegSize' (which is always equal to 128).<br>My question is: when looking for profitable vectorizable trees,<br>wouldn't it make more sense to set variable VF based (also) on the<br>value returned by TTI::getRegisterBitWidth()? I think this could have<br>a positive effect if the target supports for example 256bit vectors<br>(so, AVX/AVX2 and AVX512f). That said, I am not an expert of<br>SLPVectorizer and therefore I may have misunderstood the behavior of<br>that method :-)<br><br>On Wed, Feb 12, 2014 at 3:34 AM, Nadav Rotem <<a href="mailto:nrotem@apple.com">nrotem@apple.com</a>> wrote:<br><blockquote type="cite">Hi Andrea,<br><br>You added a new kind of OperandValueKind and you connected the cost model computation, but you forgot one big piece: the vectorizer! The vectorizer needs to use the new OperandValueKind in order to efficiently estimate the cost of the shifts. The patches you sent LGTM, but I would also like the vectorizer to use the new operand value kind. :)<br><br>Thanks,<br>Nadav<br><br><br><br>On Feb 11, 2014, at 12:29 PM, Andrea Di Biagio <<a href="mailto:andrea.dibiagio@gmail.com">andrea.dibiagio@gmail.com</a>> wrote:<br><br><blockquote type="cite">Hi Nadav and Jim,<br><br>Sorry for sending another version of the patch but I think I found how<br>to properly fix/improve the cost model.<br>For simplicity I have split the patch into two: a patch that<br>introduces the new lowering rule for vector shifts and a patch that<br>improves the x86 cost model.<br><br>Test vec_shift6.ll should now cover all the cases where vector shifts<br>are expanded into multiply instructions.<br><br>I introduced into the cost model a new 'OperandValueKind' to identify<br>operands which are constants but not constant splats.<br>I called it 'OK_NonUniformConstValue' to distinguish it from<br>'OK_UniformConstantValue' which is used for splat values.<br><br>I modified CostModel.cpp to allow returning 'OK_NonUniformConstValue'<br>for non splat operands that are instances of ConstantVector or<br>ConstantDataVector.<br><br>I verified that the cost model still produces the expected results on<br>other non x86 targets (ARM and PPC).<br>On the X86 backend, I modified X86TargetTransformInfo to produce the<br>'expected' cost for vector shift left instructions that can be lowered<br>as a vector multiply.<br><br>Finally I added a new test to verify that the output of opt<br>'-cost-model -analyze' is valid in the following configurations: SSE2,<br>SSE4.1, AVX, AVX2.<br><br>I didn't add a cost model for AVX512f, but I think (if it is ok for<br>you) that this can be done as a separate patch.<br><br>Please let me know what you think.<br><br>Thanks,<br>Andrea<br><br>On Fri, Feb 7, 2014 at 8:51 PM, Andrea Di Biagio<br><<a href="mailto:andrea.dibiagio@gmail.com">andrea.dibiagio@gmail.com</a>> wrote:<br><blockquote type="cite">Hi Jim and Nadav,<br><br>here is a new version of the patch.<br><br>The cost model is able to deal with most of the cases where the second<br>operand of a SHL is of kind OK_UniformConstantValue.<br>As far as I can see, the only missing rule is for the case where we<br>have a v16i16 SHL in AVX2.<br>I added a rule in X86TTI::getArithmeticInstrCost to update the cost<br>for that specific case.<br><br>One of the problems I have encountered is that method 'getOperandInfo'<br>in CostModel either returns OK_AnyValue or OK_UniformConstantValue (I<br>didn't find any case where we return OK_UniformValue..).<br>All the shifts optimized by my patch are shifts where the second<br>operand is a ConstantDataVector but not a splat. Therefore, function<br>getOperandInfo in CostModel.cpp will always return OK_AnyValue for<br>them.<br><br>In conclusion, I am not sure if I have modified that part correctly.<br>Please let me know if you had something else in mind and in case what<br>should be the correct way to improve that part.<br><br>I investigated other opportunties for applying this transformation to<br>other vector types.<br>This new patch improves my previous patch since we now know how to<br>expand a packed v16i16 shift left into a single AVX2 vpmullw when the<br>shift amount is a constant build_vector.<br><br>Without AVX2 support, a packed v16i16 shift left is usually decomposed<br>into two 128-bits shifts. Each new shift is then properly expanded<br>into a multiply instruction (pmullw) thanks to the new transformation<br>introduced by this patch.<br><br>The backend already knows how to efficiently lower v8i32 shifts with<br>AVX2: v8i32 shifts are expanded into VPSLLV instructions.<br>Also, the backend already knows how to emit the AVX512f version of<br>VPSLLVD and VPSLLVQ in the case of v1632 and v8i64 vectors.<br>AVX512f seems to benefit alot from this new transformation (you can<br>see it if you compare the output of test avx_shift6.ll when run for<br>AVX512 before and after applying my patch).<br><br>A vector shift left by constant v32i16 build_vector is initally split<br>into two v16i16 shifts during type-legalization. Eventually the new<br>algorithm converts the resulting shifts into multiply instructions.<br><br>See new test-cases avx_shift6.ll for more details.<br><br>Please let me know what you think.<br><br>Thanks,<br>Andrea<br><br>On Thu, Feb 6, 2014 at 11:11 PM, Andrea Di Biagio<br><<a href="mailto:andrea.dibiagio@gmail.com">andrea.dibiagio@gmail.com</a>> wrote:<br><blockquote type="cite">Hi Jim and Nadav,<br><br>thanks for the feedback!<br><br>On Thu, Feb 6, 2014 at 10:33 PM, Nadav Rotem <<a href="mailto:nrotem@apple.com">nrotem@apple.com</a>> wrote:<br><blockquote type="cite">Please remember to update the x86 cost model in X86TTI::getArithmeticInstrCost. In this function you should be able to check if the LHS is a constant.<br></blockquote><br>Ok, I will update the cost model.<br><br><blockquote type="cite">On Feb 6, 2014, at 2:26 PM, Jim Grosbach <<a href="mailto:grosbach@apple.com">grosbach@apple.com</a>> wrote:<br><br><blockquote type="cite">Hi Andrea,<br><br>This is a very nice improvement, but should do a bit more, I believe.<br><br>AVX2 adds 256-bit wide vector versions of these instructions, so if AVX2 is available, the same transformation should be applied to v16i16 and v8i32 shifts. Worth looking to see if AVX512 extends<br><br>The test cases should check that when compiling for AVX, the VEX prefixed form of the instructions are generated instead of the SSE versions.<br><br></blockquote></blockquote><br>Sure, I will send a new version of the patch that also improves AVX2<br>code generation in the case of v16i16 and v8i32. I'll also have a look<br>at AVX512 to identify cases that can also be improved.<br><br>On my previous email I forgot to mention that this patch would also<br>fix bug 18478 (<a href="http://llvm.org/bugs/show_bug.cgi?id=18478">http://llvm.org/bugs/show_bug.cgi?id=18478</a>)<br><br>Thanks again for your time.<br>Andrea<br><br><blockquote type="cite"><br><blockquote type="cite"><blockquote type="cite">Hi,<br><br>This patch teaches the backend how to efficiently lower a packed<br>vector shift left into a packed vector multiply if the vector of shift<br>counts is known to be constant (i.e. a constant build_vector).<br><br>Instead of expanding a packed shift into a sequence of scalar shifts,<br>the backend should try (when possible) to convert the vector shift<br>into a vector multiply.<br><br>Before this patch, a shift of a MVT::v8i16 vector by a build_vector of<br>constants was always scalarized into a long sequence of "vector<br>extracts + scalar shifts + vector insert".<br>With this patch, if there is SSE2 support, we emit a single vector multiply.<br><br>The new x86 test 'vec_shift6.ll' contains some examples of code that<br>are affected by this patch.<br><br>Please let me know if ok to submit.<br><br>Thanks,<br>Andrea Di Biagio<br>SN Systems - Sony Computer Entertainment Group<br><patch.diff>_______________________________________________<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></blockquote><br></blockquote></blockquote></blockquote><patch-cost-model.diff><patch-v2.diff><br></blockquote><br></blockquote><span><patch-cost-model-v2.diff></span><span><patch-v2.diff></span></div></blockquote></div><br></body></html>