<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hi Andrea,<div><br></div><div>Thanks for the quick follow-up.</div><div>LGTM.</div><div><br></div><div>Cheers,</div><div>-Quentin</div><div><br><div style=""><div>On Oct 17, 2014, at 2:20 AM, 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;">On Thu, Oct 16, 2014 at 11:17 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br><blockquote type="cite"><br><blockquote type="cite">On Oct 16, 2014, at 2:50 PM, Andrea Di Biagio <<a href="mailto:andrea.dibiagio@gmail.com">andrea.dibiagio@gmail.com</a>> wrote:<br><br>Hi Quentin,<br><br>On Thu, Oct 16, 2014 at 7:49 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br><blockquote type="cite">Hi Andrea,<br><br>The general direction looks good, just a couple of questions:<br>Since we are matching integer types, shouldn’t we use the integer variant:<br>MOVNTD?<br></blockquote><br>The only integer variant available would be movntdq (SSE2) and<br>vmovntdq (AVX) which can be used to store quad-words.<br>Since we are storing all zeros in the destination, then I think in<br>this case it should be ok to use it.<br>In case, then I would probably need to guard the pattern that selects<br>'movntdq' with a 'let Predicates = [HasSSE2] in'.<br><br>I noticed however that if we use the integer variant MOVNTDQmr, the<br>backend would eventually convert it back to a MOVNTPSmr in the attempt<br>to fix-up the execution dependency (I guess that is because the zero<br>vector is generated by a xorps).<br></blockquote><br>I was expected this :).<br>The thing I had in mind was, since we are using integer type I was expecting that in the general case the computation leading to this store would have the same type. Therefore, I thought it was better to directly generate the instruction that is in the right domain than relying on the domain fixer for that.<br></blockquote><br>I agree that emitting the instruction in the right domain is the best<br>thing to do.<br>I attached a new patch that uses the integer variant.<br><br>To clarify,<br>I was just a bit confused here because I didn't expect the backend to<br>always emit a xorps for the vector of all zeros.<br>In the example below, no matter what variant of non-temporal store we<br>use, we always end up generating a (v)xorps:<br><br>Example:<br>///<br>define void @test2(<4 x i32>* %dst) {<br> store <4 x i32> zeroinitializer, <4 x i32>* %dst, align 16, !nontemporal !1<br> ret void<br>}<br>///<br><br>Before Post-RA pseudo instruction expansion we have ( with -mcpu=corei7):<br> %XMM0<def> = V_SET0<br> MOVNTDQmr %RDI<kill>, 1, %noreg, 0, %noreg, %XMM0<kill>;<br>mem:ST16[%dst](nontemporal)<br> RETQ<br><br>During Post-RA pseudo instruction expansion pass that sequence is converted to:<br> %XMM0<def,tied1> = XORPSrr %XMM0<undef,tied0>, %XMM0<undef><br> MOVNTDQmr %RDI<kill>, 1, %noreg, 0, %noreg, %XMM0<kill>;<br>mem:ST16[%dst](nontemporal)<br> RETQ<br><br>This eventually triggers the execution domain fix:<br> %XMM0<def,tied1> = XORPSrr %XMM0<undef,tied0>, %XMM0<undef><br> MOVNTPSmr %RDI<kill>, 1, %noreg, 0, %noreg, %XMM0<kill>;<br>mem:ST16[%dst](nontemporal)<br> RETQ<br><br><blockquote type="cite"><br><blockquote type="cite"><br><blockquote type="cite"><br>Moreover, wouldn’t it make sense to have a more general pattern for this?<br>I.e., we could use VR128 instead of immAllZerosV, couldn’t we?<br></blockquote><br>Originally I tried to fix ths bug using VR128 instead of immAllZerosV.<br>However, in all my experiments, the only case I found where the<br>backend wrongly selected a normal store instead of a non-temporal<br>store was the case where a zero vector is used as value in input. This<br>is why eventually I decided to explicitly use immAllZerosV.<br></blockquote><br>I see the rational, I just wanted to be sure we caught all the cases.<br><br><blockquote type="cite"><br>That said, If I replace immAllZerosV with VR128 it would work.<br>However, I don't think (correct me if I am wrong) we would be able to<br>use the integer variant MOVNTDQmr anymore, since the input vector may<br>not be a zero vector.<br></blockquote><br>What is the problem with that?<br>Unless I am mistaken, we are storing a 128-bits value and the way it is interpreted is pointless, isn’t it?<br></blockquote><br>Ah yes, you are right. There is no problem with it.<br>Sorry for the confusion.<br><br><blockquote type="cite"><br>That makes me wonder, why the actual patterns for MOVNT do not work here?<br>We should be able to do implicit conversions from v4i32 to v2i64 (i.e., anything that stay on VR128).<br></blockquote><br>Before this patch, we only had a single pattern per non-temporal store<br>definition.<br><br>For example, in the case of VMOVNTDQmr we had:<br><br>let ExeDomain = SSEPackedInt in<br>def VMOVNTDQmr : VPDI<0xE7, MRMDestMem, (outs),<br> (ins f128mem:$dst, VR128:$src),<br> "movntdq\t{$src, $dst|$dst, $src}",<br> [(alignednontemporalstore (v2i64 VR128:$src),<br> addr:$dst)],<br> IIC_SSE_MOVNT>, VEX;<br><br>My understanding is that the explicit usage of 'v2i64' for the<br>VMOVNTDQmr is intentional since it allows to pattern match other<br>flavors of non-temporal stores when the type is not v2i64.<br>This is also the reason why the actual patterns do not work here and<br>we need the extra patterns from my patch.<br><br>What about this new patch?<br>The main differences w.r.t. the previous patch are:<br>- For v4i32 non-temporal stores, MOVNTDQmr is now selected instead of<br>MOVNTPSmr. This would keep the computation in the integer domain.<br>- The new patterns are now more generic as they allow VR128 to be<br>used instead of immAllZerosV.<br><br>Please let me know if this new version of the patch looks good to you.<br><br>Thanks again for your time,<br>Andrea<br><br><blockquote type="cite"><br>-Quentin<br><br><blockquote type="cite"><br>What do you think?<br><br>Thanks,<br>Andrea<br><br><blockquote type="cite"><br>Thanks,<br>-Quentin<br><br>On Oct 16, 2014, at 10:41 AM, Andrea Di Biagio <<a href="mailto:andrea.dibiagio@gmail.com">andrea.dibiagio@gmail.com</a>><br>wrote:<br><br>Hi,<br><br>This is a fix for PR19370.<br>Currently the x86 backend wrongly selects a a normal vector store<br>instead of a non-temporal store if the value to store is a vector of<br>all zeros.<br><br>Small reproducible:<br><br>;;<br>define void @test(<4 x float>* %dst) {<br>store <4 x float> zeroinitializer, <4 x float>* %dst, align 16,<br>!nontemporal !1<br>ret void<br>}<br><br>!1 = metadata !{i32 1}<br>;;<br><br>llc (-mtriple=x86_64-unknown-unknown -mcpu=corei7-avx) generates:<br>vxorps %xmm0, %xmm0, %xmm0<br>vmovaps %xmm0, (%rdi)<br>retq<br><br>Instead, llc should generate:<br>vxorps %xmm0, %xmm0, %xmm0<br>vmovntps %xmm0, (%rdi)<br>retq<br><br>In this example, the vector of all zeros is legalized to a zero vector<br>of type v4i32.<br>However, ISel doesn't have a rule to select a MOVNTPSmr when the<br>source is not a vector of float elements. So, it eventually (wrongly)<br>falls back to selecting a normal store.<br><br>This patch fixes the problem adding extra ISel patterns to cover that<br>particular corner case.<br><br>Please let me know if ok to commit.<br><br>Thanks!<br>Andrea<br><patch-nontemporal.diff><br><br><br></blockquote></blockquote><br></blockquote><span><patch-v2.diff></span></div></blockquote></div><br></div></body></html>