[X86][SSE] Vector integer/float conversion memory folding

[RKSimon]

Summary

Added a missing memory folding relationship for the CVTDQ2PS instruction (and its AVX variants) - we can safely fold these (but not the CVTDQ2PD versions which have a register/memory size discrepancy in the source operand). I've added a test case demonstrating that stack folding now works.

Also fixed an issue with the VCVTTPD2DQ / VCVTTPS2DQ instructions being incorrectly put in the 2 source operand folding tables instead of the 1 source operand.

Finally, tidied up some entries in the folding tables so that they are under the correct comment section (they were categorised as AVX2 instructions when they're AVX1).

[qcolombet]

Hi Simon,

Thanks for fixing this.

Could you add a test case for the second issue you fixed?

Also, could you make three patches, one for each concern?

Thanks,
-Quentin

[RKSimon]

I'll split the cvttp*todq fix from the cvtdq2ps patch and add extra tests to the avx1-stack-reload-folding.ll

BTW I could have added AVX-512 equivalents as well but have no hardware to test it on....

Does the third part of the patch need review? Its pretty benign.

[qcolombet]

Does the third part of the patch need review? Its pretty benign.

No, go for it :).

Thanks,
-Quentin

[RKSimon]

Removed the CVTTP*2DQ fixes - I will create a separate patch for this shortly.

[spatel]

test/CodeGen/X86/avx1-stack-reload-folding.ll

17–23 ↗

(On Diff #15488)

Is there a guideline regarding IR test cases with types that aren't specified by the ABI?

Ie, is the lowering of 128-element vectors to legal x86 types stable? Would it be better to define a struct here?

[qcolombet]

test/CodeGen/X86/avx1-stack-reload-folding.ll

17–23 ↗

(On Diff #15488)

Same remark as Sanjay: would it be possible to have a simpler test case, especially with legal types?

[RKSimon]

Updated test so that we don't use non-native vector types as arguments - now uses pointers to load/store instead.

We need to keep to this basic pattern to ensure we spill a lot to stack and the 'big vectors' approach drastically helps makes this simple to understand. Using a more basic load/cvt/store pattern would mean that it wouldn't be the memory folding code thats being tested - the load-execute version of the instruction from the tables would be matched much earlier instead.

[qcolombet]

Hi Simon,

We need to keep to this basic pattern to ensure we spill a lot to stack and the 'big vectors' approach drastically helps makes this simple to understand. Using a more basic load/cvt/store pattern would mean that it wouldn't be the memory folding code thats being tested - the load-execute version of the instruction from the tables would be matched much earlier instead.

I was afraid of that. Could you add a comment explaining that at the being of the tests?

Please commit with this change.

Thanks,
-Quentin

[RKSimon]

Closed by commit rL221407 (authored by @RKSimon).

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

static const X86OpTblEntry OpTbl1[] = {
     { X86::IMUL64rri32,     X86::IMUL64rmi32,         0 },
     { X86::IMUL64rri8,      X86::IMUL64rmi8,          0 },
     { X86::Int_COMISDrr,    X86::Int_COMISDrm,        0 },
     { X86::Int_COMISSrr,    X86::Int_COMISSrm,        0 },
     { X86::CVTSD2SI64rr,    X86::CVTSD2SI64rm,        0 },
     { X86::CVTSD2SIrr,      X86::CVTSD2SIrm,          0 },
     { X86::CVTSS2SI64rr,    X86::CVTSS2SI64rm,        0 },
     { X86::CVTSS2SIrr,      X86::CVTSS2SIrm,          0 },
+    { X86::CVTDQ2PSrr,      X86::CVTDQ2PSrm,          TB_ALIGN_16 },
     { X86::CVTTPD2DQrr,     X86::CVTTPD2DQrm,         TB_ALIGN_16 },
     { X86::CVTTPS2DQrr,     X86::CVTTPS2DQrm,         TB_ALIGN_16 },
     { X86::Int_CVTTSD2SI64rr,X86::Int_CVTTSD2SI64rm,  0 },
     { X86::Int_CVTTSD2SIrr, X86::Int_CVTTSD2SIrm,     0 },
     { X86::Int_CVTTSS2SI64rr,X86::Int_CVTTSS2SI64rm,  0 },
     { X86::Int_CVTTSS2SIrr, X86::Int_CVTTSS2SIrm,     0 },
     { X86::Int_UCOMISDrr,   X86::Int_UCOMISDrm,       0 },
     { X86::Int_UCOMISSrr,   X86::Int_UCOMISSrm,       0 },
static const X86OpTblEntry OpTbl1[] = {
     { X86::VCVTTSS2SI64rr,  X86::VCVTTSS2SI64rm,      0 },
     { X86::Int_VCVTTSS2SI64rr,X86::Int_VCVTTSS2SI64rm,0 },
     { X86::VCVTTSS2SIrr,    X86::VCVTTSS2SIrm,        0 },
     { X86::Int_VCVTTSS2SIrr,X86::Int_VCVTTSS2SIrm,    0 },
     { X86::VCVTSD2SI64rr,   X86::VCVTSD2SI64rm,       0 },
     { X86::VCVTSD2SIrr,     X86::VCVTSD2SIrm,         0 },
     { X86::VCVTSS2SI64rr,   X86::VCVTSS2SI64rm,       0 },
     { X86::VCVTSS2SIrr,     X86::VCVTSS2SIrm,         0 },
+    { X86::VCVTDQ2PSrr,     X86::VCVTDQ2PSrm,         0 },
     { X86::VMOV64toPQIrr,   X86::VMOVQI2PQIrm,        0 },
     { X86::VMOV64toSDrr,    X86::VMOV64toSDrm,        0 },
     { X86::VMOVAPDrr,       X86::VMOVAPDrm,           TB_ALIGN_16 },
     { X86::VMOVAPSrr,       X86::VMOVAPSrm,           TB_ALIGN_16 },
     { X86::VMOVDDUPrr,      X86::VMOVDDUPrm,          0 },
     { X86::VMOVDI2PDIrr,    X86::VMOVDI2PDIrm,        0 },
     { X86::VMOVDI2SSrr,     X86::VMOVDI2SSrm,         0 },
     { X86::VMOVDQArr,       X86::VMOVDQArm,           TB_ALIGN_16 },
static const X86OpTblEntry OpTbl1[] = {
     { X86::VRSQRTPSr_Int,   X86::VRSQRTPSm_Int,       0 },
     { X86::VSQRTPDr,        X86::VSQRTPDm,            0 },
     { X86::VSQRTPSr,        X86::VSQRTPSm,            0 },
     { X86::VUCOMISDrr,      X86::VUCOMISDrm,          0 },
     { X86::VUCOMISSrr,      X86::VUCOMISSrm,          0 },
     { X86::VBROADCASTSSrr,  X86::VBROADCASTSSrm,      TB_NO_REVERSE },
 
     // AVX 256-bit foldable instructions
+    { X86::VCVTDQ2PSYrr,    X86::VCVTDQ2PSYrm,        0 },
     { X86::VMOVAPDYrr,      X86::VMOVAPDYrm,          TB_ALIGN_32 },
     { X86::VMOVAPSYrr,      X86::VMOVAPSYrm,          TB_ALIGN_32 },
     { X86::VMOVDQAYrr,      X86::VMOVDQAYrm,          TB_ALIGN_32 },
     { X86::VMOVUPDYrr,      X86::VMOVUPDYrm,          0 },
     { X86::VMOVUPSYrr,      X86::VMOVUPSYrm,          0 },
     { X86::VPERMILPDYri,    X86::VPERMILPDYmi,        0 },
     { X86::VPERMILPSYri,    X86::VPERMILPSYmi,        0 },
     { X86::VRCPPSYr,        X86::VRCPPSYm,            0 },

llvm/trunk/test/CodeGen/X86/avx1-stack-reload-folding.ll

 ; RUN: llc -O3 -disable-peephole -mcpu=corei7-avx -mattr=+avx < %s | FileCheck %s
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-unknown"
 
-; Function Attrs: nounwind readonly uwtable
-define <32 x double> @_Z14vstack_foldDv32_dS_(<32 x double> %a, <32 x double> %b) #0 {
-  %1 = fadd <32 x double> %a, %b
-  %2 = fsub <32 x double> %a, %b
-  %3 = fmul <32 x double> %1, %2
-  ret <32 x double> %3
+; Stack reload folding tests - we use the 'big vectors' pattern to guarantee spilling to stack.
+;
+; Many of these tests are primarily to check memory folding with specific instructions. Using a basic
+; load/cvt/store pattern to test for this would mean that it wouldn't be the memory folding code thats
+; being tested - the load-execute version of the instruction from the tables would be matched instead.
 
-  ;CHECK-NOT:  vmovapd {{.*#+}} 32-byte Reload
+define void @stack_fold_vmulpd(<64 x double>* %a, <64 x double>* %b, <64 x double>* %c) {
   ;CHECK:       vmulpd {{[0-9]*}}(%rsp), {{%ymm[0-9][0-9]*}}, {{%ymm[0-9][0-9]*}} {{.*#+}} 32-byte Folded Reload
-  ;CHECK-NOT:  vmovapd {{.*#+}} 32-byte Reload
+  %1 = load <64 x double>* %a
+  %2 = load <64 x double>* %b
+  %3 = fadd <64 x double> %1, %2
+  %4 = fsub <64 x double> %1, %2
+  %5 = fmul <64 x double> %3, %4
+  store <64 x double> %5, <64 x double>* %c
+  ret void
+}
+
+define void @stack_fold_cvtdq2ps(<128 x i32>* %a, <128 x i32>* %b, <128 x float>* %c) {
+  ;CHECK:   vcvtdq2ps {{[0-9]*}}(%rsp), {{%ymm[0-9][0-9]*}} {{.*#+}} 32-byte Folded Reload
+  %1 = load <128 x i32>* %a
+  %2 = load <128 x i32>* %b
+  %3 = and <128 x i32> %1, %2
+  %4 = xor <128 x i32> %1, %2
+  %5 = sitofp <128 x i32> %3 to <128 x float>
+  %6 = sitofp <128 x i32> %4 to <128 x float>
+  %7 = fadd <128 x float> %5, %6
+  store <128 x float> %7, <128 x float>* %c
+  ret void
 }