[PATCH] D89701: Fix TypeSize warning in redundant store elimination

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm marked an inline comment as done.
peterwaller-arm added a comment.

In D89701#2338897 <https://reviews.llvm.org/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?



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17328
           // of a vector, so bail out if MemoryVT is scalable.
+          !ST->getMemoryVT().isScalableVector() &&
           !ST1->getMemoryVT().isScalableVector()) {
----------------
sdesmalen wrote:
> 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.
Agreed.


================
Comment at: llvm/test/CodeGen/AArch64/Redundantstore.ll:1
-; RUN: llc < %s -O3 -mtriple=aarch64-eabi | FileCheck %s 
+; RUN: llc < %s -O3 -mtriple=aarch64-eabi 2>&1 | FileCheck %s
 
----------------
sdesmalen wrote:
> 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```
> 
Ah, nifty thanks.


================
Comment at: llvm/test/CodeGen/AArch64/Redundantstore.ll:38
+; 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>*
----------------
sdesmalen wrote:
> Missing CHECK lines to check that the optimisation is not applied?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89701/new/

https://reviews.llvm.org/D89701