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

[andreadb]

Summary

This patch teaches X86FastISel how to select scalar float/double convert operations using AVX instructions.

Before this patch, X86FastISel always selected legacy SSE instructions for FPExt (from float to double) and FPTrunc (from double to float).

For example:
\code

define double @foo(float %f) {
  %conv = fpext float %f to double
  ret double %conv
}

\end code

Before (with -mattr=+avx -fast-isel), X86FastIsel selected a CVTSS2SDrr which is
legacy SSE:

cvtss2sd %xmm0, %xmm0

With this patch (with -mattr=+avx -fast-isel), X86FastIsel selects a VCVTSS2SDrr instead:

vcvtss2sd %xmm0, %xmm0, %xmm0

Added test fast-isel-fptrunc-fpext.ll to check both the register-register and the register-memory float/double conversion variants.

Please let me know if ok to submit.

Thanks!
Andrea

[mkuper]

Looks good in general, with a couple of comments.

lib/Target/X86/X86FastISel.cpp

2019 ↗

(On Diff #19407)

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.

2046 ↗

(On Diff #19407)

Perhaps this is now large enough to move the common code from the two functions into a helper?

test/CodeGen/X86/fast-isel-fptrunc-fpext.ll

25 ↗

(On Diff #19407)

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)

[andreadb]

Hi Michael,

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

-Andrea

lib/Target/X86/X86FastISel.cpp

2019 ↗

(On Diff #19407)

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.

2046 ↗

(On Diff #19407)

Good point. I'll try to move the common code into a helper function.

test/CodeGen/X86/fast-isel-fptrunc-fpext.ll

25 ↗

(On Diff #19407)

Sure, I will change the tests.

[andreadb]

Hi Michael,

Here is a new version of the patch which hopefully addresses all your comments.
These are the main differences with respect to the previous patch:

1. I removed all the uses of IMPLICIT_DEF registers. If the target has AVX, the same register is passed to both input operands.
2. I changed the tests so that on SSE, we check that the instruction generated doesn't have a vex prefix.
3. I tried to factor out common code into a separate helper named 'X86SelectFPExtOrFPTrunc'.

Please let me know if ok to submit.

Thanks again for your time,
Andrea

[mkuper]

Thanks, Andrea!
LGTM.

[andreadb]

Thanks Michael!
committed revision 228682.

llvm/trunk/lib/Target/X86/X86FastISel.cpp

private:
   bool X86FastEmitSSESelect(MVT RetVT, const Instruction *I);
 
   bool X86FastEmitPseudoSelect(MVT RetVT, const Instruction *I);
 
   bool X86SelectSelect(const Instruction *I);
 
   bool X86SelectTrunc(const Instruction *I);
 
+  bool X86SelectFPExtOrFPTrunc(const Instruction *I, unsigned Opc,
+                               const TargetRegisterClass *RC);
+
   bool X86SelectFPExt(const Instruction *I);
   bool X86SelectFPTrunc(const Instruction *I);
 
   const X86InstrInfo *getInstrInfo() const {
     return Subtarget->getInstrInfo();
   }
   const X86TargetMachine *getTargetMachine() const {
     return static_cast<const X86TargetMachine *>(&TM);
bool X86FastISel::X86SelectSelect(const Instruction *I) {
   // Fall-back to pseudo conditional move instructions, which will be later
   // converted to control-flow.
   if (X86FastEmitPseudoSelect(RetVT, I))
     return true;
 
   return false;
 }
 
-bool X86FastISel::X86SelectFPExt(const Instruction *I) {
-  // fpext from float to double.
-  if (X86ScalarSSEf64 &&
-      I->getType()->isDoubleTy()) {
-    const Value *V = I->getOperand(0);
-    if (V->getType()->isFloatTy()) {
-      unsigned OpReg = getRegForValue(V);
-      if (OpReg == 0) return false;
-      unsigned ResultReg = createResultReg(&X86::FR64RegClass);
-      BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
-              TII.get(X86::CVTSS2SDrr), ResultReg)
-        .addReg(OpReg);
-      updateValueMap(I, ResultReg);
-      return true;
-    }
+// Helper method used by X86SelectFPExt and X86SelectFPTrunc.
+bool X86FastISel::X86SelectFPExtOrFPTrunc(const Instruction *I,
+                                          unsigned TargetOpc,
+                                          const TargetRegisterClass *RC) {
+  assert((I->getOpcode() == Instruction::FPExt ||
+          I->getOpcode() == Instruction::FPTrunc) &&
+         "Instruction must be an FPExt or FPTrunc!");
+
+  unsigned OpReg = getRegForValue(I->getOperand(0));
+  if (OpReg == 0)
+    return false;
+
+  unsigned ResultReg = createResultReg(RC);
+  MachineInstrBuilder MIB;
+  MIB = BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(TargetOpc),
+                ResultReg);
+  if (Subtarget->hasAVX())
+    MIB.addReg(OpReg);
+  MIB.addReg(OpReg);
+
+bool X86FastISel::X86SelectFPExt(const Instruction *I) {
+  if (X86ScalarSSEf64 && I->getType()->isDoubleTy() &&
+      I->getOperand(0)->getType()->isFloatTy()) {
+    // fpext from float to double.
+    unsigned Opc = Subtarget->hasAVX() ? X86::VCVTSS2SDrr : X86::CVTSS2SDrr;
+    return X86SelectFPExtOrFPTrunc(I, Opc, &X86::FR64RegClass);
   }
 
   return false;
 }
 
 bool X86FastISel::X86SelectFPTrunc(const Instruction *I) {
-  if (X86ScalarSSEf64) {
-    if (I->getType()->isFloatTy()) {
-      const Value *V = I->getOperand(0);
-      if (V->getType()->isDoubleTy()) {
-        unsigned OpReg = getRegForValue(V);
-        if (OpReg == 0) return false;
-        unsigned ResultReg = createResultReg(&X86::FR32RegClass);
-        BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
-                TII.get(X86::CVTSD2SSrr), ResultReg)
-          .addReg(OpReg);
-        updateValueMap(I, ResultReg);
-        return true;
-      }
-    }
+  if (X86ScalarSSEf64 && I->getType()->isFloatTy() &&
+      I->getOperand(0)->getType()->isDoubleTy()) {
+    // fptrunc from double to float.
+    unsigned Opc = Subtarget->hasAVX() ? X86::VCVTSD2SSrr : X86::CVTSD2SSrr;
+    return X86SelectFPExtOrFPTrunc(I, Opc, &X86::FR32RegClass);
   }
 
   return false;
 }
 
 bool X86FastISel::X86SelectTrunc(const Instruction *I) {
   EVT SrcVT = TLI.getValueType(I->getOperand(0)->getType());
   EVT DstVT = TLI.getValueType(I->getType());

llvm/trunk/test/CodeGen/X86/fast-isel-fptrunc-fpext.ll

+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+sse2 -fast-isel | FileCheck %s --check-prefix=ALL --check-prefix=SSE
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx -fast-isel | FileCheck %s --check-prefix=ALL --check-prefix=AVX
+;
+; Verify that fast-isel doesn't select legacy SSE instructions on targets that
+; feature AVX.
+;
+; Test cases are obtained from the following code snippet:
+; ///
+; double single_to_double_rr(float x) {
+;   return (double)x;
+; }
+; float double_to_single_rr(double x) {
+;   return (float)x;
+; }
+; double single_to_double_rm(float *x) {
+;   return (double)*x;
+; }
+; float double_to_single_rm(double *x) {
+;   return (float)*x;
+; }
+; ///
+
+define double @single_to_double_rr(float %x) {
+; ALL-LABEL: single_to_double_rr:
+; SSE-NOT: vcvtss2sd
+; AVX: vcvtss2sd %xmm0, %xmm0, %xmm0
+; ALL: ret
+entry:
+  %conv = fpext float %x to double
+  ret double %conv
+}
+
+define float @double_to_single_rr(double %x) {
+; ALL-LABEL: double_to_single_rr:
+; SSE-NOT: vcvtsd2ss
+; AVX: vcvtsd2ss %xmm0, %xmm0, %xmm0
+; ALL: ret
+entry:
+  %conv = fptrunc double %x to float
+  ret float %conv
+}
+
+define double @single_to_double_rm(float* %x) {
+; ALL-LABEL: single_to_double_rm:
+; SSE: cvtss2sd (%rdi), %xmm0
+; AVX: vmovss (%rdi), %xmm0
+; AVX-NEXT: vcvtss2sd %xmm0, %xmm0, %xmm0
+; ALL-NEXT: ret
+entry:
+  %0 = load float* %x, align 4
+  %conv = fpext float %0 to double
+  ret double %conv
+}
+
+define float @double_to_single_rm(double* %x) {
+; ALL-LABEL: double_to_single_rm:
+; SSE: cvtsd2ss (%rdi), %xmm0
+; AVX: vmovsd (%rdi), %xmm0
+; AVX-NEXT: vcvtsd2ss %xmm0, %xmm0, %xmm0
+; ALL-NEXT: ret
+entry:
+  %0 = load double* %x, align 8
+  %conv = fptrunc double %0 to float
+  ret float %conv
+}