Hi Elena, <div><br /></div><div>isSplatVector does not do what the name says. It checks it there is a splat constant. It should have a different name. <br /><br /></div><div><div>+    N0IsConst = isSplatVector(N0.getNode(), ConstValue0);</div><div>+    N1IsConst = isSplatVector(N1.getNode(), ConstValue1);</div><div>   }</div><div>+  else {</div><div>+    N0IsConst = dyn_cast<ConstantSDNode>(N0) != 0;</div><div>+    Const</div></div><div><br /></div><div>The else should be on the same line. </div><div><br /></div><div>Nadav</div><div><br /></div><div>On 06/25/13, <b class="name">"Demikhovsky, Elena" </b> <elena.demikhovsky@intel.com> wrote:<br /><blockquote cite="mid: <A0DC88CEB3010344830D52D66533DA8E023C3EA4@HASMSX104.ger.corp.intel.com" class="iwcQuote" style="border-left: 1px solid #00F; padding-left: 13px; margin-left: 0;" type="cite"><div class="mimepart multipart mixed">Can anybody, please, take a look at this patch, I'd like to commit it.<br /><br />Thank you.<br /><br />-  Elena<br /><br /><br />-----Original Message-----<br />From: Demikhovsky, Elena <br />Sent: Sunday, June 23, 2013 14:00<br />To: Andrea_DiBiagio@sn.scee.net<br />Cc: Benjamin Kramer; llvm-commits@cs.uiuc.edu; llvm-commits-bounces@cs.uiuc.edu<br />Subject: RE: Please review: Optimize vector multiply on X86<br /><br />I caught your point. I also fixed the same bug on SDIV lowering. This is the new patch.<br /><br />-  Elena<br /><br /><br />-----Original Message-----<br />From: Andrea_DiBiagio@sn.scee.net [<a href="mailto:Andrea_DiBiagio@sn.scee.net]">mailto:Andrea_DiBiagio@sn.scee.net]</a><br />Sent: Thursday, June 20, 2013 16:15<br />To: Demikhovsky, Elena<br />Cc: Benjamin Kramer; llvm-commits@cs.uiuc.edu; llvm-commits-bounces@cs.uiuc.edu<br />Subject: RE: Please review: Optimize vector multiply on X86<br /><br />Hi Elena,<br /><br />> From: "Demikhovsky, Elena" <elena.demikhovsky@intel.com> Hi Andrea,<br />> <br />> You are right that 'isSplatVector' returns true if elements are not <br />> the same. But later I check the slat value for 0, -1, pow2 And in the <br />> tests I check everything:<br /><br />What I am saying is that the size in bit of the splat value returned by 'isSplatVector' matters. For example, it may be bigger than the vector element size in bits.<br /><br />Example, given the following <4 x i64> constant vectors:<br />a) <i64 2, i64 0, i64 0, i64 0><br />b) <i64 2, i64 0, i64 2, i64 0><br />c) <i64 2, i64 2, i64 2, i64 2><br /><br />They are all considered to be valid vector splats according to 'isSplatVector' and the splat value is the same for all of them. What changes however is the SplatBitSize:<br />a) SplatValue = 2 ;; SplatBitSize = 256;<br />b) SplatValue = 2 ;; SplatBitSize = 128;<br />c) SplatValue = 2 ;; SplatBitSize = 64;<br /><br />> ==== Multiple 2 gives add<br />> +define <8 x i32> @mul_const1(<8 x i32> %x) {<br />> +  %y = mul <8 x i32> %x, <i32 2, i32 2, i32 2, i32 2, i32 2, i32 2,<br />> i32 2, i32 2><br />> +  ret <8 x i32> %y<br />> +}<br /><br />In the example above:<br />if you substitute the second operand of the multiply instruction with any of the following constant vectors:<br /><br /> - <i32 2, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0><br /> - <i32 2, i32 0, i32 0, i32 0, i32 2, i32 0, i32 0, i32 0><br /> - <i32 2, i32 0, i32 2, i32 0, i32 2, i32 0, i32 2, i32 0><br /><br />You will find out that your new combine transformation always optimizes the multiply into an add, and that causes incorrect codegen.<br />That is because the compiler applies the following sequence of rules:<br /> 1) fold (mul x, (1 << c)) -> x << c  (in DAGCombiner::visitMul())<br /> 2) (shl V, 1) -> add V, V (in X86ISelLowering.cpp, function<br />PerformSHLCombine())<br /><br />> === non-spalt int value is not optimized<br />> +define <8 x i32> @mul_const6(<8 x i32> %x) {<br />> +  %y = mul <8 x i32> %x, <i32 0, i32 0, i32 0, i32 2, i32 0, i32 2,<br />> i32 0, i32 0><br />> +  ret <8 x i32> %y<br />> +}<br /><br />That is true for the example above but it is not always true in general.<br />If you change the code above into:<br /><br />define <8 x i32> @mul_const6(<8 x i32> %x) {<br />  %y = mul <8 x i32> %x, <i32 2, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0,<br />i32 0><br />  ret <8 x i32> %y<br />}<br /><br />You will get that the multiply is wrongly converted into an add.<br /><br />Andrea Di Biagio<br />SN Systems - Sony Computer Entertainment Group<br /><br /><br />**********************************************************************<br />This email and any files transmitted with it are confidential and intended solely for the use of the individual or entity to whom they are addressed. <br />If you have received this email in error please notify postmaster@scee.net This footnote also confirms that this email message has been checked for all known viruses.<br />Sony Computer Entertainment Europe Limited Registered Office: 10 Great Marlborough Street, London W1F 7LP, United Kingdom Registered in England: 3277793<br />**********************************************************************<br /><br />P Please consider the environment before printing this e-mail<br />---------------------------------------------------------------------<br />Intel Israel (74) Limited<br /><br />This e-mail and any attachments may contain confidential material for<br />the sole use of the intended recipient(s). Any review or distribution<br />by others is strictly prohibited. If you are not the intended<br />recipient, please contact the sender and delete all copies.<br /></div></blockquote></div>