[x86] enable storeOfVectorConstantIsCheap() target hook

[spatel]

Summary

This allows vector-sized store merging of constants in DAGCombiner using the existing code in MergeConsecutiveStores(). All of the twisted logic that decides exactly what vector operations are legal and fast for each particular CPU are handled separately in there using the appropriate hooks.

Some notes:

1. For the motivating tests in merge-store-constants.ll, we already produce the same vector code in IR via the SLP vectorizer. So this is just providing a backend backstop for code that doesn't go through that pass (-O1). More details in PR24449:

https://bugs.llvm.org/show_bug.cgi?id=24449 (this change should be the last step to resolve that bug)

2. At the minimum vector size limit (16-bytes), we're trading two 8-byte scalar immediate stores for one 16-byte constant pool load and one 16-byte store (eg, fold-vector-sext-crash2.ll::test_sext1). I think that's a reasonable trade-off because offloading any work to vector registers should ease pressure on register-starved scalar code, but let me know if there are other considerations. We could adjust this in the hook by returning true only for >2*max scalar size, so we know there would an instruction reduction.

3. There's a likely regression in vector-sext-crash2.ll::test_zext1 and mod128.ll where we materialize a constant in scalar and then send it over to the vector unit. I know we have some bug reports related to that. A quick scan turned up:

https://bugs.llvm.org/show_bug.cgi?id=26301
...but there are probably others.

[zvi]

Intuitively, it seems to me that choosing a minimum threshold, as suggested in note 2, is a better option.
Another concern for store-merging in general: are we more susceptible to losing store-to-load forwarding? I know that Intel pre-Nehalem processors had some limitations that were later improved. Sorry i can't recall the full details from the top of my head. Will look later at the Optimization Manual for the info.

[spatel]

In D37451#863494, @zvi wrote:

Intuitively, it seems to me that choosing a minimum threshold, as suggested in note 2, is a better option.

Yes, that should be a more clear win. It should also sidestep the scalar imm -> move to vector -> store regressions we see here.

Another concern for store-merging in general: are we more susceptible to losing store-to-load forwarding? I know that Intel pre-Nehalem processors had some limitations that were later improved. Sorry i can't recall the full details from the top of my head. Will look later at the Optimization Manual for the info.

I can see that concern in general, but we're only dealing with constant stores in this patch, so I would hope there's no problem rematerializing constants.

[spatel]

Patch updated:
Don't enable vector store of constants unless we can replace more than 2 scalar stores. This avoids the borderline cases and sidesteps the regressions in the earlier rev.

big_nonzero_16_bytes() for x86-64 shows a different merging problem. We can't directly store 64-bit immediates, so we have to materialize the constants in registers and then store as a separate instruction. The test is trying to store four 32-bit imms, so we probably shouldn't have done any merging there?

[spatel]

In D37451#863708, @spatel wrote:

big_nonzero_16_bytes() for x86-64 shows a different merging problem. We can't directly store 64-bit immediates, so we have to materialize the constants in registers and then store as a separate instruction. The test is trying to store four 32-bit imms, so we probably shouldn't have done any merging there?

On 2nd thought, that doesn't make sense. We probably should merge that case, but the hook doesn't allow us to distinguish constant values. So we don't know when an immediate can be placed directly in the store or not.

I think it's ok to only handle the larger cases in this patch and make that potential enhancement a follow-up. Fixing that might also solve the scalar imm to vector to memory bug.

[spatel]

Patch updated:
On 3rd thought... :)
We can simplify the code and handle the previously mentioned test on 64-bit too. I've added another test to show the potential follow-up improvement.

[spatel]

Ping.

[RKSimon]

LGTM

llvm/trunk/lib/Target/X86/X86ISelLowering.h

public:
 
     bool convertSelectOfConstantsToMath(EVT VT) const override;
 
     /// Return true if EXTRACT_SUBVECTOR is cheap for this result type
     /// with this index.
     bool isExtractSubvectorCheap(EVT ResVT, EVT SrcVT,
                                  unsigned Index) const override;
 
+    bool storeOfVectorConstantIsCheap(EVT MemVT, unsigned NumElem,
+                                      unsigned AddrSpace) const override {
+      // If we can replace more than 2 scalar stores, there will be a reduction
+      // in instructions even after we add a vector constant load.
+      return NumElem > 2;
+    }
+
     /// Intel processors have a unified instruction and data cache
     const char * getClearCacheBuiltinName() const override {
       return nullptr; // nothing to do, move along.
     }
 
     unsigned getRegisterByName(const char* RegName, EVT VT,
                                SelectionDAG &DAG) const override;
 

llvm/trunk/test/CodeGen/X86/avx512-regcall-Mask.ll

define x86_regcallcc i64 @test_argv64i1(<64 x i1> %x0, <64 x i1> %x1, <64 x i1> %x2,
   %add9 = add i64 %add8, %y9
   %add10 = add i64 %add9, %y10
   %add11 = add i64 %add10, %y11
   %add12 = add i64 %add11, %y12
   ret i64 %add12
 }
 
 ; X32-LABEL:  caller_argv64i1:
-; X32:        movl    $2, %eax
-; X32:        movl    $1, %ecx
-; X32:        movl    $2, %edx
-; X32:        movl    $1, %edi
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        pushl    ${{1|2}}
-; X32:        call{{.*}}   _test_argv64i1
-        
+; X32:  pushl %edi
+; X32:  subl  $88, %esp
+; X32:  vmovaps __xmm@00000001000000020000000100000002, %xmm0 # xmm0 = [2,1,2,1]
+; X32:  vmovups %xmm0, 64(%esp)
+; X32:  vmovaps LCPI1_1, %zmm0          # zmm0 = [2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1]
+; X32:  vmovups %zmm0, (%esp)
+; X32:  movl  $1, 84(%esp)
+; X32:  movl  $2, 80(%esp)
+; X32:  movl  $2, %eax
+; X32:  movl  $1, %ecx
+; X32:  movl  $2, %edx
+; X32:  movl  $1, %edi
+; X32:  vzeroupper
+; X32:  calll _test_argv64i1
 ; WIN64-LABEL: caller_argv64i1:
 ; WIN64:       movabsq    $4294967298, %rax
 ; WIN64:       movq   %rax, (%rsp)
 ; WIN64:       movq   %rax, %rcx
 ; WIN64:       movq   %rax, %rdx
 ; WIN64:       movq   %rax, %rdi
 ; WIN64:       movq   %rax, %rsi
 ; WIN64:       movq   %rax, %r8

llvm/trunk/test/CodeGen/X86/merge-store-constants.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=i686-unknown-unknown   -mattr=avx | FileCheck %s --check-prefix=X32
 ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=avx | FileCheck %s --check-prefix=X64
 
 define void @big_nonzero_16_bytes(i32* nocapture %a) {
 ; X32-LABEL: big_nonzero_16_bytes:
 ; X32:       # BB#0:
 ; X32-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; X32-NEXT:    movl $1, (%eax)
-; X32-NEXT:    movl $2, 4(%eax)
-; X32-NEXT:    movl $3, 8(%eax)
-; X32-NEXT:    movl $4, 12(%eax)
+; X32-NEXT:    vmovaps {{.*#+}} xmm0 = [1,2,3,4]
+; X32-NEXT:    vmovups %xmm0, (%eax)
 ; X32-NEXT:    retl
 ;
 ; X64-LABEL: big_nonzero_16_bytes:
 ; X64:       # BB#0:
-; X64-NEXT:    movabsq $8589934593, %rax # imm = 0x200000001
-; X64-NEXT:    movq %rax, (%rdi)
-; X64-NEXT:    movabsq $17179869187, %rax # imm = 0x400000003
-; X64-NEXT:    movq %rax, 8(%rdi)
+; X64-NEXT:    vmovaps {{.*#+}} xmm0 = [1,2,3,4]
+; X64-NEXT:    vmovups %xmm0, (%rdi)
 ; X64-NEXT:    retq
   %arrayidx1 = getelementptr inbounds i32, i32* %a, i64 1
   %arrayidx2 = getelementptr inbounds i32, i32* %a, i64 2
   %arrayidx3 = getelementptr inbounds i32, i32* %a, i64 3
 
   store i32 1, i32* %a, align 4
   store i32 2, i32* %arrayidx1, align 4
   store i32 3, i32* %arrayidx2, align 4
   store i32 4, i32* %arrayidx3, align 4
   ret void
 }
 
+; TODO: We assumed that two 64-bit stores were better than 1 vector load and 1 vector store.
+; But if the 64-bit constants can't be represented as sign-extended 32-bit constants, then
+; it takes extra instructions to do this in scalar.
+
+define void @big_nonzero_16_bytes_big64bit_constants(i64* nocapture %a) {
+; X32-LABEL: big_nonzero_16_bytes_big64bit_constants:
+; X32:       # BB#0:
+; X32-NEXT:    movl {{[0-9]+}}(%esp), %eax
+; X32-NEXT:    vmovaps {{.*#+}} xmm0 = [1,1,1,3]
+; X32-NEXT:    vmovups %xmm0, (%eax)
+; X32-NEXT:    retl
+;
+; X64-LABEL: big_nonzero_16_bytes_big64bit_constants:
+; X64:       # BB#0:
+; X64-NEXT:    movabsq $4294967297, %rax # imm = 0x100000001
+; X64-NEXT:    movq %rax, (%rdi)
+; X64-NEXT:    movabsq $12884901889, %rax # imm = 0x300000001
+; X64-NEXT:    movq %rax, 8(%rdi)
+; X64-NEXT:    retq
+  %arrayidx1 = getelementptr inbounds i64, i64* %a, i64 1
+
+  store i64 4294967297, i64* %a
+  store i64 12884901889, i64* %arrayidx1
+  ret void
+}
+
 ; Splats may be an opportunity to use a broadcast op.
 
 define void @big_nonzero_32_bytes_splat(i32* nocapture %a) {
 ; X32-LABEL: big_nonzero_32_bytes_splat:
 ; X32:       # BB#0:
 ; X32-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; X32-NEXT:    movl $42, (%eax)
-; X32-NEXT:    movl $42, 4(%eax)
-; X32-NEXT:    movl $42, 8(%eax)
-; X32-NEXT:    movl $42, 12(%eax)
-; X32-NEXT:    movl $42, 16(%eax)
-; X32-NEXT:    movl $42, 20(%eax)
-; X32-NEXT:    movl $42, 24(%eax)
-; X32-NEXT:    movl $42, 28(%eax)
+; X32-NEXT:    vmovaps {{.*#+}} ymm0 = [42,42,42,42,42,42,42,42]
+; X32-NEXT:    vmovups %ymm0, (%eax)
+; X32-NEXT:    vzeroupper
 ; X32-NEXT:    retl
 ;
 ; X64-LABEL: big_nonzero_32_bytes_splat:
 ; X64:       # BB#0:
-; X64-NEXT:    movabsq $180388626474, %rax # imm = 0x2A0000002A
-; X64-NEXT:    movq %rax, (%rdi)
-; X64-NEXT:    movq %rax, 8(%rdi)
-; X64-NEXT:    movq %rax, 16(%rdi)
-; X64-NEXT:    movq %rax, 24(%rdi)
+; X64-NEXT:    vmovaps {{.*#+}} ymm0 = [42,42,42,42,42,42,42,42]
+; X64-NEXT:    vmovups %ymm0, (%rdi)
+; X64-NEXT:    vzeroupper
 ; X64-NEXT:    retq
   %arrayidx1 = getelementptr inbounds i32, i32* %a, i64 1
   %arrayidx2 = getelementptr inbounds i32, i32* %a, i64 2
   %arrayidx3 = getelementptr inbounds i32, i32* %a, i64 3
   %arrayidx4 = getelementptr inbounds i32, i32* %a, i64 4
   %arrayidx5 = getelementptr inbounds i32, i32* %a, i64 5
   %arrayidx6 = getelementptr inbounds i32, i32* %a, i64 6
   %arrayidx7 = getelementptr inbounds i32, i32* %a, i64 7

 }
 
 ; Verify that we choose the best-sized store(s) for each chunk.
 
 define void @big_nonzero_63_bytes(i8* nocapture %a) {
 ; X32-LABEL: big_nonzero_63_bytes:
 ; X32:       # BB#0:
 ; X32-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; X32-NEXT:    movl $0, 4(%eax)
-; X32-NEXT:    movl $1, (%eax)
-; X32-NEXT:    movl $0, 12(%eax)
-; X32-NEXT:    movl $2, 8(%eax)
-; X32-NEXT:    movl $0, 20(%eax)
-; X32-NEXT:    movl $3, 16(%eax)
-; X32-NEXT:    movl $0, 28(%eax)
-; X32-NEXT:    movl $4, 24(%eax)
-; X32-NEXT:    movl $0, 36(%eax)
-; X32-NEXT:    movl $5, 32(%eax)
-; X32-NEXT:    movl $0, 44(%eax)
-; X32-NEXT:    movl $6, 40(%eax)
+; X32-NEXT:    vmovaps {{.*#+}} ymm0 = [1,0,2,0,3,0,4,0]
+; X32-NEXT:    vmovups %ymm0, (%eax)
+; X32-NEXT:    vmovaps {{.*#+}} xmm0 = [5,0,6,0]
+; X32-NEXT:    vmovups %xmm0, 32(%eax)
 ; X32-NEXT:    movl $0, 52(%eax)
 ; X32-NEXT:    movl $7, 48(%eax)
 ; X32-NEXT:    movl $8, 56(%eax)
 ; X32-NEXT:    movw $9, 60(%eax)
 ; X32-NEXT:    movb $10, 62(%eax)
+; X32-NEXT:    vzeroupper
 ; X32-NEXT:    retl
 ;
 ; X64-LABEL: big_nonzero_63_bytes:
 ; X64:       # BB#0:
-; X64-NEXT:    movq $1, (%rdi)
-; X64-NEXT:    movq $2, 8(%rdi)
-; X64-NEXT:    movq $3, 16(%rdi)
-; X64-NEXT:    movq $4, 24(%rdi)
+; X64-NEXT:    vmovaps {{.*#+}} ymm0 = [1,2,3,4]
+; X64-NEXT:    vmovups %ymm0, (%rdi)
 ; X64-NEXT:    movq $5, 32(%rdi)
 ; X64-NEXT:    movq $6, 40(%rdi)
 ; X64-NEXT:    movq $7, 48(%rdi)
 ; X64-NEXT:    movl $8, 56(%rdi)
 ; X64-NEXT:    movw $9, 60(%rdi)
 ; X64-NEXT:    movb $10, 62(%rdi)
+; X64-NEXT:    vzeroupper
 ; X64-NEXT:    retq
   %a8 = bitcast i8* %a to i64*
   %arrayidx8 = getelementptr inbounds i64, i64* %a8, i64 1
   %arrayidx16 = getelementptr inbounds i64, i64* %a8, i64 2
   %arrayidx24 = getelementptr inbounds i64, i64* %a8, i64 3
   %arrayidx32 = getelementptr inbounds i64, i64* %a8, i64 4
   %arrayidx40 = getelementptr inbounds i64, i64* %a8, i64 5
   %arrayidx48 = getelementptr inbounds i64, i64* %a8, i64 6