DAGCombiner: Replace store of FP constant after attemping store merges

[arsenm]

Summary

If storing multiple FP constants, some subset of the stores
would be replaced with integers due to visit order, so
MergeConsecutiveStores would only partially merge these.

Alternatively, it possibly would be better to do this after LegalTypes

[spatel]

This appears to solve PR24654:
https://llvm.org/bugs/show_bug.cgi?id=24654

Can you split this patch into an NFC refactoring to add replaceStoreOfFPConstant() and then submit the logic changes as a separate patch? That would make it easier to read.

The AMDGPU FIXME test addition could be a separate commit too.

[RKSimon]

In D12698#246307, @spatel wrote:

This appears to solve PR24654:
https://llvm.org/bugs/show_bug.cgi?id=24654

Matt - please can you add the x86/ppc test cases that are mentioned in PR24654?

[arsenm]

Only do move in this patch. Add x86/ppc tests. I'll commit the refactor separately

[spatel]

LGTM - just a small comment on the test case settings.

test/CodeGen/PowerPC/vector-merge-store-fp-constants.ll

1

We shouldn't need -mattr=altivec or darwin here; that was excess in the bug report.
Also, for this test and the next one: if you specify the triple, does it change anything to also specify the arch?

[arsenm]

test/CodeGen/PowerPC/vector-merge-store-fp-constants.ll

1

I don't think it does anything, but I usually specify both

[arsenm]

r248168-248169

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (Align <= OrigAlign &&
                           ST->isNonTemporal(), OrigAlign,
                           ST->getAAInfo());
   }
 
   // Turn 'store undef, Ptr' -> nothing.
   if (Value.getOpcode() == ISD::UNDEF && ST->isUnindexed())
     return Chain;
 
-  // Turn 'store float 1.0, Ptr' -> 'store int 0x12345678, Ptr'
-  //
-  // Make sure to do this only after attempting to merge stores in order to
-  //  avoid changing the types of some subset of stores due to visit order,
-  //  preventing their merging.
-  if (isa<ConstantFPSDNode>(Value)) {
-    if (SDValue NewSt = replaceStoreOfFPConstant(ST))
-      return NewSt;
-  }
-
   // Try to infer better alignment information than the store already has.
   if (OptLevel != CodeGenOpt::None && ST->isUnindexed()) {
     if (unsigned Align = DAG.InferPtrAlignment(Ptr)) {
       if (Align > ST->getAlignment()) {
         SDValue NewStore =
                DAG.getTruncStore(Chain, SDLoc(N), Value,
                                  Ptr, ST->getPointerInfo(), ST->getMemoryVT(),
                                  ST->isVolatile(), ST->isNonTemporal(), Align,
do {
       EverChanged |= Changed;
       if (!Changed) break;
     } while (ST->getOpcode() != ISD::DELETED_NODE);
 
     if (EverChanged)
       return SDValue(N, 0);
   }
 
+  // Turn 'store float 1.0, Ptr' -> 'store int 0x12345678, Ptr'
+  //
+  // Make sure to do this only after attempting to merge stores in order to
+  //  avoid changing the types of some subset of stores due to visit order,
+  //  preventing their merging.
+  if (isa<ConstantFPSDNode>(Value)) {
+    if (SDValue NewSt = replaceStoreOfFPConstant(ST))
+      return NewSt;
+  }
+
   return ReduceLoadOpStoreWidth(N);
 }
 
 SDValue DAGCombiner::visitINSERT_VECTOR_ELT(SDNode *N) {
   SDValue InVec = N->getOperand(0);
   SDValue InVal = N->getOperand(1);
   SDValue EltNo = N->getOperand(2);
   SDLoc dl(N);

test/CodeGen/AMDGPU/merge-stores.ll

define void @merge_global_store_4_constants_i32(i32 addrspace(1)* %out) #0 {
   store i32 123, i32 addrspace(1)* %out.gep.1
   store i32 456, i32 addrspace(1)* %out.gep.2
   store i32 333, i32 addrspace(1)* %out.gep.3
   store i32 1234, i32 addrspace(1)* %out
   ret void
 }
 
 ; GCN-LABEL: {{^}}merge_global_store_4_constants_f32_order:
-; XGCN: buffer_store_dwordx4
-; GCN: buffer_store_dword v
-; GCN: buffer_store_dword v
-; GCN: buffer_store_dwordx2 v
+; GCN: buffer_store_dwordx4
 define void @merge_global_store_4_constants_f32_order(float addrspace(1)* %out) #0 {
   %out.gep.1 = getelementptr float, float addrspace(1)* %out, i32 1
   %out.gep.2 = getelementptr float, float addrspace(1)* %out, i32 2
   %out.gep.3 = getelementptr float, float addrspace(1)* %out, i32 3
 
   store float 8.0, float addrspace(1)* %out
   store float 1.0, float addrspace(1)* %out.gep.1
   store float 2.0, float addrspace(1)* %out.gep.2
   store float 4.0, float addrspace(1)* %out.gep.3
   ret void
 }
 
-; First store is out of order. Because of order of combines, the
-; consecutive store fails because only some of the stores have been
-; replaced with integer constant stores, and then won't merge because
-; the types are different.
-
+; First store is out of order.
 ; GCN-LABEL: {{^}}merge_global_store_4_constants_f32:
-; XGCN: buffer_store_dwordx4
-; GCN: buffer_store_dword v
-; GCN: buffer_store_dword v
-; GCN: buffer_store_dword v
-; GCN: buffer_store_dword v
+; GCN: buffer_store_dwordx4
 define void @merge_global_store_4_constants_f32(float addrspace(1)* %out) #0 {
   %out.gep.1 = getelementptr float, float addrspace(1)* %out, i32 1
   %out.gep.2 = getelementptr float, float addrspace(1)* %out, i32 2
   %out.gep.3 = getelementptr float, float addrspace(1)* %out, i32 3
 
   store float 1.0, float addrspace(1)* %out.gep.1
   store float 2.0, float addrspace(1)* %out.gep.2
   store float 4.0, float addrspace(1)* %out.gep.3
   store float 8.0, float addrspace(1)* %out
   ret void
 }
 
+; FIXME: Should be able to merge this
+; GCN-LABEL: {{^}}merge_global_store_4_constants_mixed_i32_f32:
+; XGCN: buffer_store_dwordx4
+; GCN: buffer_store_dword
+; GCN: buffer_store_dword
+; GCN: buffer_store_dword
+; GCN: buffer_store_dword
+; GCN: s_endpgm
+define void @merge_global_store_4_constants_mixed_i32_f32(float addrspace(1)* %out) #0 {
+  %out.gep.1 = getelementptr float, float addrspace(1)* %out, i32 1
+  %out.gep.2 = getelementptr float, float addrspace(1)* %out, i32 2
+  %out.gep.3 = getelementptr float, float addrspace(1)* %out, i32 3
+
+  %out.gep.1.bc = bitcast float addrspace(1)* %out.gep.1 to i32 addrspace(1)*
+  %out.gep.3.bc = bitcast float addrspace(1)* %out.gep.3 to i32 addrspace(1)*
+
+  store i32 11, i32 addrspace(1)* %out.gep.1.bc
+  store float 2.0, float addrspace(1)* %out.gep.2
+  store i32 17, i32 addrspace(1)* %out.gep.3.bc
+  store float 8.0, float addrspace(1)* %out
+  ret void
+}
+
 ; GCN-LABEL: {{^}}merge_global_store_3_constants_i32:
 ; SI-DAG: buffer_store_dwordx2
 ; SI-DAG: buffer_store_dword
 ; SI-NOT: buffer_store_dword
 ; GCN: s_endpgm
 define void @merge_global_store_3_constants_i32(i32 addrspace(1)* %out) #0 {
   %out.gep.1 = getelementptr i32, i32 addrspace(1)* %out, i32 1
   %out.gep.2 = getelementptr i32, i32 addrspace(1)* %out, i32 2

test/CodeGen/PowerPC/vector-merge-store-fp-constants.ll

+; RUN: llc -march=ppc64 -mtriple=ppc64-apple-darwin -mattr=+altivec < %s | FileCheck %s
+
+; CHECK-LABEL: {{^}}_merge_8_float_zero_stores:
+; CHECK: li [[ZEROREG:r[0-9]+]], 0
+; CHECK-DAG: std [[ZEROREG]], 0([[PTR:r[0-9]+]])
+; CHECK-DAG: std [[ZEROREG]], 8([[PTR]])
+; CHECK-DAG: std [[ZEROREG]], 16([[PTR]])
+; CHECK-DAG: std [[ZEROREG]], 24([[PTR]])
+; CHECK: blr
+define void @merge_8_float_zero_stores(float* %ptr) {
+  %idx0 = getelementptr float, float* %ptr, i64 0
+  %idx1 = getelementptr float, float* %ptr, i64 1
+  %idx2 = getelementptr float, float* %ptr, i64 2
+  %idx3 = getelementptr float, float* %ptr, i64 3
+  %idx4 = getelementptr float, float* %ptr, i64 4
+  %idx5 = getelementptr float, float* %ptr, i64 5
+  %idx6 = getelementptr float, float* %ptr, i64 6
+  %idx7 = getelementptr float, float* %ptr, i64 7
+  store float 0.0, float* %idx0, align 4
+  store float 0.0, float* %idx1, align 4
+  store float 0.0, float* %idx2, align 4
+  store float 0.0, float* %idx3, align 4
+  store float 0.0, float* %idx4, align 4
+  store float 0.0, float* %idx5, align 4
+  store float 0.0, float* %idx6, align 4
+  store float 0.0, float* %idx7, align 4
+  ret void
+}

test/CodeGen/X86/vector-merge-store-fp-constants.ll

+; RUN: llc -march=x86-64 -mtriple=x86_64-unknown-unknown < %s | FileCheck -check-prefix=DEFAULTCPU -check-prefix=ALL %s
+; RUN: llc -march=x86-64 -mcpu=x86-64 -mtriple=x86_64-unknown-unknown < %s | FileCheck -check-prefix=X8664CPU -check-prefix=ALL %s
+
+
+; ALL-LABEL: {{^}}merge_8_float_zero_stores:
+
+; DEFAULTCPU-DAG: movq $0, ([[PTR:%[a-z]+]])
+; DEFAULTCPU-DAG: movq $0, 8([[PTR]])
+; DEFAULTCPU-DAG: movq $0, 16([[PTR]])
+; DEFAULTCPU-DAG: movq $0, 24([[PTR]])
+
+; X8664CPU: xorps [[ZEROREG:%xmm[0-9]+]], [[ZEROREG]]
+; X8664CPU-DAG: movups [[ZEROREG]], ([[PTR:%[a-z]+]])
+; X8664CPU-DAG: movups [[ZEROREG]], 16([[PTR:%[a-z]+]])
+
+; ALL: retq
+define void @merge_8_float_zero_stores(float* %ptr) {
+  %idx0 = getelementptr float, float* %ptr, i64 0
+  %idx1 = getelementptr float, float* %ptr, i64 1
+  %idx2 = getelementptr float, float* %ptr, i64 2
+  %idx3 = getelementptr float, float* %ptr, i64 3
+  %idx4 = getelementptr float, float* %ptr, i64 4
+  %idx5 = getelementptr float, float* %ptr, i64 5
+  %idx6 = getelementptr float, float* %ptr, i64 6
+  %idx7 = getelementptr float, float* %ptr, i64 7
+  store float 0.0, float* %idx0, align 4
+  store float 0.0, float* %idx1, align 4
+  store float 0.0, float* %idx2, align 4
+  store float 0.0, float* %idx3, align 4
+  store float 0.0, float* %idx4, align 4
+  store float 0.0, float* %idx5, align 4
+  store float 0.0, float* %idx6, align 4
+  store float 0.0, float* %idx7, align 4
+  ret void
+}