[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

17329

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 ↗

(On Diff #299046)

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 ↗

(On Diff #299046)

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

17329

Agreed.

llvm/test/CodeGen/AArch64/Redundantstore.ll

1 ↗

(On Diff #299046)

Ah, nifty thanks.

38 ↗

(On Diff #299046)

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 ↗

(On Diff #299046)

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

20

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

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/sve-redundant-store.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -O2 -mtriple=aarch64-linux-gnu -mattr=+sve < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; If this check fails please read test/CodeGen/AArch64/README for instructions on how to resolve it.
+; WARN-NOT: warning: {{.*}}TypeSize is not scalable
+
+; #include <arm_sve.h>
+; #include <stdint.h>
+;
+; void redundant_store(uint32_t *p, svint32_t v) {
+;     *p = 1;
+;     *(svint32_t *)p = v;
+; }
+
+; Update me: Until dead store elimination is improved in DAGCombine, this will contain a redundant store.
+;
+define void @redundant_store(i32* nocapture %p, <vscale x 4 x i32> %v) {
+; CHECK-LABEL: redundant_store:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    mov w8, #1
+; CHECK-NEXT:    ptrue p0.s
+; CHECK-NEXT:    str w8, [x0]
+; CHECK-NEXT:    st1w { z0.s }, p0, [x0]
+; CHECK-NEXT:    ret
+  store i32 1, i32* %p, align 4
+  %1 = bitcast i32* %p to <vscale x 4 x i32>*
+  store <vscale x 4 x i32> %v, <vscale x 4 x i32>* %1, align 16
+  ret void
+}