[llvm] 66d64aa - [GlobalISel] Fix a store-merging bug due to use of >= instead of >.

[Amara Emerson via llvm-commits]

Author: Amara Emerson
Date: 2023-02-19T15:55:50-08:00
New Revision: 66d64aac36a69d08509d5a00ef302ddf783e939f

URL: https://github.com/llvm/llvm-project/commit/66d64aac36a69d08509d5a00ef302ddf783e939f
DIFF: https://github.com/llvm/llvm-project/commit/66d64aac36a69d08509d5a00ef302ddf783e939f.diff

LOG: [GlobalISel] Fix a store-merging bug due to use of >= instead of >.

This fixes a corner case where we would skip doing an alias check because of a
>= vs > bug, due to the presence of a non-aliasing instruction, in this case
the load %safeld.

Fixes issue #59376

Added: 
    

Modified: 
    llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
    llvm/test/CodeGen/AArch64/GlobalISel/store-merging.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp b/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
index bdcb6432c48e1..32d60c4311450 100644
--- a/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
@@ -400,7 +400,9 @@ bool LoadStoreOpt::doSingleStoreMerge(SmallVectorImpl<GStore *> &Stores) {
   auto NewStore =
       Builder.buildStore(WideReg, FirstStore->getPointerReg(), *WideMMO);
   (void) NewStore;
-  LLVM_DEBUG(dbgs() << "Created merged store: " << *NewStore);
+  LLVM_DEBUG(dbgs() << "Merged " << Stores.size()
+                    << " stores into merged store: " << *NewStore);
+  LLVM_DEBUG(for (auto *MI : Stores) dbgs() << "  " << *MI;);
   NumStoresMerged += Stores.size();
 
   MachineOptimizationRemarkEmitter MORE(*MF, nullptr);
@@ -445,20 +447,19 @@ bool LoadStoreOpt::processMergeCandidate(StoreMergeCandidate &C) {
     for (auto AliasInfo : reverse(C.PotentialAliases)) {
       MachineInstr *PotentialAliasOp = AliasInfo.first;
       unsigned PreCheckedIdx = AliasInfo.second;
-      if (static_cast<unsigned>(Idx) > PreCheckedIdx) {
-        // Need to check this alias.
-        if (GISelAddressing::instMayAlias(CheckStore, *PotentialAliasOp, *MRI,
-                                          AA)) {
-          LLVM_DEBUG(dbgs() << "Potential alias " << *PotentialAliasOp
-                            << " detected\n");
-          return true;
-        }
-      } else {
+      if (static_cast<unsigned>(Idx) < PreCheckedIdx) {
         // Once our store index is lower than the index associated with the
         // potential alias, we know that we've already checked for this alias
         // and all of the earlier potential aliases too.
         return false;
       }
+      // Need to check this alias.
+      if (GISelAddressing::instMayAlias(CheckStore, *PotentialAliasOp, *MRI,
+                                        AA)) {
+        LLVM_DEBUG(dbgs() << "Potential alias " << *PotentialAliasOp
+                          << " detected\n");
+        return true;
+      }
     }
     return false;
   };

diff  --git a/llvm/test/CodeGen/AArch64/GlobalISel/store-merging.ll b/llvm/test/CodeGen/AArch64/GlobalISel/store-merging.ll
index 0f525248b0688..7a9eee6f30b67 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/store-merging.ll
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/store-merging.ll
@@ -296,3 +296,30 @@ entry:
   store atomic i32 0, ptr %add.ptr.i.i38 release, align 4
   ret void
 }
+
+; Here store of 9 and 15 can be merged, but the store of 0 prevents the store
+; of 5 from being considered. This checks a corner case where we would skip
+; doing an alias check because of a >= vs > bug, due to the presence of a
+; non-aliasing instruction, in this case the load %safeld.
+define i32 @test_alias_3xs16(ptr %ptr, ptr %ptr2, ptr %ptr3, ptr noalias %safe_ptr) {
+; CHECK-LABEL: test_alias_3xs16:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    mov x10, #9
+; CHECK-NEXT:    mov x8, x0
+; CHECK-NEXT:    ldr w0, [x3]
+; CHECK-NEXT:    mov w9, #5
+; CHECK-NEXT:    movk x10, #14, lsl #32
+; CHECK-NEXT:    str w9, [x8, #4]
+; CHECK-NEXT:    strh wzr, [x8, #4]
+; CHECK-NEXT:    str x10, [x8, #8]
+; CHECK-NEXT:    ret
+  %safeld = load i32, ptr %safe_ptr
+  %addr2 = getelementptr i32, ptr %ptr, i64 1
+  store i32 5, ptr %addr2
+  store i16 0, ptr %addr2 ; aliases directly with store above.
+  %addr3 = getelementptr i32, ptr %ptr, i64 2
+  store i32 9, ptr %addr3
+  %addr4 = getelementptr i32, ptr %ptr, i64 3
+  store i32 14, ptr %addr4
+  ret i32 %safeld
+}