[SVE][CodeGen][DAGCombiner] Fix TypeSize warning in redundant store elimination

[peterwaller-arm]

Summary

The modified code in visitSTORE was missing a scalable vector check, and still
using the now deprecated implicit cast of TypeSize to uint64_t through the
overloaded operator. This patch fixes these issues.

This brings the logic in line with the comment on the context line immediately
above the added precondition.

Add a test in sve-redundant-store.ll that the warning is not triggered.

[sdesmalen]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

17328

Now that we have the isKnownGE functions in TypeSize, it should be possible to check if the second store is greater than the first one, and if so, still mark the first one as redundant. But that may be more something to move into BaseIndexOffset::contains instead, and probably not something to do in this patch.

llvm/test/CodeGen/AArch64/Redundantstore.ll

1

In other tests we've done this with:

; RUN: llc [commands here] < %s 2>%t | FileCheck %s
; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t

With a reference to some README file in case people run into this failure:

; If this check fails please read test/CodeGen/AArch64/README for instructions on how to resolve it.
; WARN-NOT: warning

38

Missing CHECK lines to check that the optimisation is not applied?

[david-arm]

Is it perhaps worth renaming the test to have sve in the name, i.e. sve-redundant-stores.ll? We've been following this convention for other tests.

[peterwaller-arm]

In D89701#2338897, @david-arm wrote:

Is it perhaps worth renaming the test to have sve in the name, i.e. sve-redundant-stores.ll? We've been following this convention for other tests.

The other test in this file (to do with redundant stores) is not sve-specific. Should I put the new test in a separate file?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

17328

Agreed.

llvm/test/CodeGen/AArch64/Redundantstore.ll

1

Ah, nifty thanks.

38

Is this desirable? For the time being I was trying to prevent the reappearance of the warning. It seems to me that the optimization is legitimate.

[peterwaller-arm]

Address review comments.

• Move test into its own file sve-redundant-stores.ll
• Use WARN-NOT idiom

[sdesmalen]

llvm/test/CodeGen/AArch64/Redundantstore.ll

38

I think it's desirable because the code will now do something different (i.e. it will no longer apply the transformation), so that's worth verifying.

llvm/test/CodeGen/AArch64/sve-redundant-store.ll

19 ↗

(On Diff #299083)

nit: you can also pass this as operand to the function.

[peterwaller-arm]

Address review comments:

• Update llc test using utils/update_llc_test_checks.py.
• Pass vector as operand to function to shrink test.

[peterwaller-arm]

Put back subject [tags] stripped by git am.

[sdesmalen]

Thanks for the changes @peterwaller-arm, LGTM!

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

 //===- DAGCombiner.cpp - Implement a DAG node combiner --------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
if (ST->isUnindexed() && ST->isSimple() &&
         // same location, then the store is dead/noop.
         return Chain;
       }
 
       if (OptLevel != CodeGenOpt::None && ST1->hasOneUse() &&
           !ST1->getBasePtr().isUndef() &&
           // BaseIndexOffset and the code below requires knowing the size
           // of a vector, so bail out if MemoryVT is scalable.
+          !ST->getMemoryVT().isScalableVector() &&
           !ST1->getMemoryVT().isScalableVector()) {
         const BaseIndexOffset STBase = BaseIndexOffset::match(ST, DAG);
         const BaseIndexOffset ChainBase = BaseIndexOffset::match(ST1, DAG);
-        unsigned STBitSize = ST->getMemoryVT().getSizeInBits();
-        unsigned ChainBitSize = ST1->getMemoryVT().getSizeInBits();
+        unsigned STBitSize = ST->getMemoryVT().getFixedSizeInBits();
+        unsigned ChainBitSize = ST1->getMemoryVT().getFixedSizeInBits();
         // If this is a store who's preceding store to a subset of the current
         // location and no one other node is chained to that store we can
         // effectively drop the store. Do not remove stores to undef as they may
         // be used as data sinks.
         if (STBase.contains(DAG, STBitSize, ChainBase, ChainBitSize)) {
           CombineTo(ST1, ST1->getChain());
           return SDValue();
         }

llvm/test/CodeGen/AArch64/Redundantstore.ll

-; RUN: llc < %s -O3 -mtriple=aarch64-eabi | FileCheck %s 
+; RUN: llc < %s -O3 -mtriple=aarch64-eabi 2>&1 | FileCheck %s
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 @end_of_array = common global i8* null, align 8
 
+; The tests in this file should not produce a TypeSize warning.
+; CHECK-NOT: warning: {{.*}}TypeSize is not scalable
+
 ; CHECK-LABEL: @test
 ; CHECK: stur
 ; CHECK-NOT: stur
 define i8* @test(i32 %size) {
 entry:
   %0 = load i8*, i8** @end_of_array, align 8
   %conv = sext i32 %size to i64
   %and = and i64 %conv, -8
   %conv2 = trunc i64 %and to i32
   %add.ptr.sum = add nsw i64 %and, -4
   %add.ptr3 = getelementptr inbounds i8, i8* %0, i64 %add.ptr.sum
   %size4 = bitcast i8* %add.ptr3 to i32*
   store i32 %conv2, i32* %size4, align 4
   %add.ptr.sum9 = add nsw i64 %and, -4
   %add.ptr5 = getelementptr inbounds i8, i8* %0, i64 %add.ptr.sum9
   %size6 = bitcast i8* %add.ptr5 to i32*
   store i32 %conv2, i32* %size6, align 4
   ret i8* %0
 }
 
+; #include <arm_sve.h>
+; #include <stdint.h>
+;
+; void redundant_store(uint32_t *x) {
+;     *x = 1;
+;     *(svint32_t *)x = svdup_s32(0);
+; }
+
+; CHECK-LABEL: @redundant_store
+define void @redundant_store(i32* nocapture %x) local_unnamed_addr #0 {
+  %1 = bitcast i32* %x to <vscale x 4 x i32>*
+  store i32 1, i32* %x, align 4
+  %2 = tail call <vscale x 4 x i32> @llvm.aarch64.sve.dup.x.nxv4i32(i32 0)
+  store <vscale x 4 x i32> %2, <vscale x 4 x i32>* %1, align 16
+  ret void
+}
+
+declare <vscale x 4 x i32> @llvm.aarch64.sve.dup.x.nxv4i32(i32)
+
+attributes #0 = { "target-cpu"="generic" "target-features"="+neon,+sve,+v8.2a" }