Hi Andrea,<div><br></div><div>Wouldn't the tablegen patterns be problematic when we use the load result several times?</div><div>If we use it several times, then we shouldn't generate additional loads. Or do you think it doesn't matter since they should be close together, since we're generating for one BB only?</div>
<div><br></div><div>That was the reasoning behind doing it with code and guarding them with MayFoldLoad, which includes hasOneUse().</div><div><br></div><div>I'll add the AVX tests today.</div><div><br></div><div>  Filipe<br>
<br>On Friday, May 16, 2014, Andrea Di Biagio <<a href="mailto:Andrea_DiBiagio@sn.scee.net">Andrea_DiBiagio@sn.scee.net</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Filipe,<br>
<br>
I think there is a better way to fix this bug.<br>
<br>
For example, you can simply add the following ISel patterns to X86InstrSSE.td instead of adding new target combine rules in X86ISelLowering.cpp.<br>
<br>
<br>
  let Predicates = [UseSSE41] in {<br>
    def : Pat<(v4f32 (X86insertps (v4f32 VR128:$src1), (loadv4f32 addr:$src2),<br>
                  imm:$src3)),<br>
              (INSERTPSrm VR128:$src1, addr:$src2, imm:$src3)>;<br>
    def : Pat<(v4f32 (X86insertps (v4f32 VR128:$src1), (X86PShufd (v4f32<br>
                   (scalar_to_vector (loadf32 addr:$src2))), (i8 0)), imm:$src3)),<br>
              (INSERTPSrm VR128:$src1, addr:$src2, imm:$src3)>;<br>
  }<br>
<br>
  let Predicates = [UseAVX] in {<br>
    def : Pat<(v4f32 (X86insertps (v4f32 VR128:$src1), (loadv4f32 addr:$src2),<br>
                  imm:$src3)),<br>
              (VINSERTPSrm VR128:$src1, addr:$src2, imm:$src3)>;<br>
    def : Pat<(v4f32 (X86insertps (v4f32 VR128:$src1),<br>
                  (X86VBroadcast (loadf32 addr:$src2)), imm:$src3)),<br>
              (VINSERTPSrm VR128:$src1, addr:$src2, imm:$src3)>;<br>
  }<br>
<br>
I am pretty sure that the four patterns above would cover all the interesting cases.<br>
I personally prefer to have tablegen patterns instead of complicated target combine rules that would only trigger on the already legalized and immediately before instruction selection.<br>
<br>
Also, you should add tests to verify the AVX codegen as well. Currently your new tests only verifies that we do the correct thing with SSE4.1. However, your patch also affects AVX (in fact, you specifically added combine rules for the case where the inserted element comes from a vbroadcast).<br>

<br>
-Andrea<br>
<br>
<a href="http://reviews.llvm.org/D3581" target="_blank">http://reviews.llvm.org/D3581</a><br>
<br>
<br>
</blockquote></div>