[PATCH] [X86][FastISel] Avoid introducing legacy SSE instructions if the subtarget has AVX.

Andrea Di Biagio Andrea_DiBiagio at sn.scee.net
Fri Feb 6 16:20:39 PST 2015

Hi Michael,

thanks for the review. I will send a new version of the patch that addresses all your comments.


Comment at: lib/Target/X86/X86FastISel.cpp:2019
@@ +2018,3 @@
+      if (HasAVX) {
+        ImplicitDefReg = createResultReg(RC);
+        BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
mkuper wrote:
> I have the feeling just reusing OpReg here would be better. 
> It's true that having an IMPLICIT_DEF, in theory, gives the regalloc more freedom, but I'm afraid it may just cause false-dependence trouble further on if decides to choose a different register. So it may be better to just force OpReg.
> This is what the pattern for the rr version does, btw:
> ```
> def : Pat<(f64 (fextend FR32:$src)),
>     (VCVTSS2SDrr FR32:$src, FR32:$src)>, Requires<[UseAVX]>;
> ```
> The pattern for the rm version has an IMPLICIT_DEF, but in that case, there is no choice.
Right, I will remove the IMPLICIT_DEF and just use OpReg for both operands in the AVX case.
The reason why I ended up using IMPLICIT_DEF was to give a chance to FastISel to also match the 'register-memory' variants of scalar float/double convert.

Comment at: lib/Target/X86/X86FastISel.cpp:2046
@@ +2045,3 @@
+      // Avoid introducing a legacy SSE instruction if the target has AVX.
+      bool HasAVX = Subtarget->hasAVX();
+      unsigned Opc = HasAVX ? X86::VCVTSD2SSrr : X86::CVTSD2SSrr;
mkuper wrote:
> Perhaps this is now large enough to move the common code from the two functions into a helper?
Good point. I'll try to move the common code into a helper function.

Comment at: test/CodeGen/X86/fast-isel-fptrunc-fpext.ll:25
@@ +24,3 @@
+; ALL-LABEL: single_to_double_rr:
+; SSE: cvtss2sd %xmm0, %xmm0
+; AVX: vcvtss2sd %xmm0, %xmm0, %xmm0
mkuper wrote:
> If I remember correctly, this will also match "vcvtss2sd %xmm0, %xmm0, %xmm0" since there's no requirement for the match to start at the beginning of a line, and this is a partial match. Perhaps have an SSE-NOT for the v version?
> (Same applies to all testcases)
Sure, I will change the tests.



More information about the llvm-commits mailing list