From 66d64aac36a69d08509d5a00ef302ddf783e939f Mon Sep 17 00:00:00 2001 From: Amara Emerson Date: Sun, 19 Feb 2023 15:36:36 -0800 Subject: [PATCH] [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 --- llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp | 21 +++++++++-------- .../CodeGen/AArch64/GlobalISel/store-merging.ll | 27 ++++++++++++++++++++++ 2 files changed, 38 insertions(+), 10 deletions(-) diff --git a/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp b/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp index bdcb643..32d60c4 100644 --- a/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp +++ b/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp @@ -400,7 +400,9 @@ bool LoadStoreOpt::doSingleStoreMerge(SmallVectorImpl &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(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(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 0f52524..7a9eee6 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 +} -- 2.7.4