[llvm-branch-commits] [llvm] release/18.x: [GlobalISel] Fix store merging incorrectly classifying an unknown index expr as 0. (#90375) (PR #90673)
tstellar wrote: Hi @aemerson (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR. https://github.com/llvm/llvm-project/pull/90673 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] release/18.x: [GlobalISel] Fix store merging incorrectly classifying an unknown index expr as 0. (#90375) (PR #90673)
https://github.com/tstellar closed https://github.com/llvm/llvm-project/pull/90673 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] release/18.x: [GlobalISel] Fix store merging incorrectly classifying an unknown index expr as 0. (#90375) (PR #90673)
https://github.com/tstellar updated
https://github.com/llvm/llvm-project/pull/90673
>From ece9d35f1a705ab8d66895c6d985907f2b9a2c0c Mon Sep 17 00:00:00 2001
From: Amara Emerson
Date: Wed, 1 May 2024 05:42:14 +0800
Subject: [PATCH] [GlobalISel] Fix store merging incorrectly classifying an
unknown index expr as 0. (#90375)
During analysis, we incorrectly leave the offset part of an address info
struct
as zero, when in actual fact we failed to decompose it into base +
offset.
This results in incorrectly assuming that the address is adjacent to
another store
addr. To fix this we wrap the offset in an optional<> so we can
distinguish between
real zero and unknown.
Fixes issue #90242
(cherry picked from commit 19f4d68252b70c81ebb1686a5a31069eda5373de)
---
.../llvm/CodeGen/GlobalISel/LoadStoreOpt.h| 20 --
llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp | 48 --
.../AArch64/GlobalISel/store-merging.ll | 19 ++
.../AArch64/GlobalISel/store-merging.mir | 62 ---
4 files changed, 115 insertions(+), 34 deletions(-)
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h
b/llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h
index 0f20a33f3a755c..7990997835d019 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h
@@ -35,11 +35,23 @@ struct LegalityQuery;
class MachineRegisterInfo;
namespace GISelAddressing {
/// Helper struct to store a base, index and offset that forms an address
-struct BaseIndexOffset {
+class BaseIndexOffset {
+private:
Register BaseReg;
Register IndexReg;
- int64_t Offset = 0;
- bool IsIndexSignExt = false;
+ std::optional Offset;
+
+public:
+ BaseIndexOffset() = default;
+ Register getBase() { return BaseReg; }
+ Register getBase() const { return BaseReg; }
+ Register getIndex() { return IndexReg; }
+ Register getIndex() const { return IndexReg; }
+ void setBase(Register NewBase) { BaseReg = NewBase; }
+ void setIndex(Register NewIndex) { IndexReg = NewIndex; }
+ void setOffset(std::optional NewOff) { Offset = NewOff; }
+ bool hasValidOffset() const { return Offset.has_value(); }
+ int64_t getOffset() const { return *Offset; }
};
/// Returns a BaseIndexOffset which describes the pointer in \p Ptr.
@@ -89,7 +101,7 @@ class LoadStoreOpt : public MachineFunctionPass {
// order stores are writing to incremeneting consecutive addresses. So when
// we walk the block in reverse order, the next eligible store must write
to
// an offset one store width lower than CurrentLowestOffset.
-uint64_t CurrentLowestOffset;
+int64_t CurrentLowestOffset;
SmallVector Stores;
// A vector of MachineInstr/unsigned pairs to denote potential aliases that
// need to be checked before the candidate is considered safe to merge. The
diff --git a/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
b/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
index 246aa88b09acf6..ee499c41c558c3 100644
--- a/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
@@ -84,21 +84,20 @@ BaseIndexOffset GISelAddressing::getPointerInfo(Register
Ptr,
MachineRegisterInfo ) {
BaseIndexOffset Info;
Register PtrAddRHS;
- if (!mi_match(Ptr, MRI, m_GPtrAdd(m_Reg(Info.BaseReg), m_Reg(PtrAddRHS {
-Info.BaseReg = Ptr;
-Info.IndexReg = Register();
-Info.IsIndexSignExt = false;
+ Register BaseReg;
+ if (!mi_match(Ptr, MRI, m_GPtrAdd(m_Reg(BaseReg), m_Reg(PtrAddRHS {
+Info.setBase(Ptr);
+Info.setOffset(0);
return Info;
}
-
+ Info.setBase(BaseReg);
auto RHSCst = getIConstantVRegValWithLookThrough(PtrAddRHS, MRI);
if (RHSCst)
-Info.Offset = RHSCst->Value.getSExtValue();
+Info.setOffset(RHSCst->Value.getSExtValue());
// Just recognize a simple case for now. In future we'll need to match
// indexing patterns for base + index + constant.
- Info.IndexReg = PtrAddRHS;
- Info.IsIndexSignExt = false;
+ Info.setIndex(PtrAddRHS);
return Info;
}
@@ -114,15 +113,16 @@ bool GISelAddressing::aliasIsKnownForLoadStore(const
MachineInstr ,
BaseIndexOffset BasePtr0 = getPointerInfo(LdSt1->getPointerReg(), MRI);
BaseIndexOffset BasePtr1 = getPointerInfo(LdSt2->getPointerReg(), MRI);
- if (!BasePtr0.BaseReg.isValid() || !BasePtr1.BaseReg.isValid())
+ if (!BasePtr0.getBase().isValid() || !BasePtr1.getBase().isValid())
return false;
int64_t Size1 = LdSt1->getMemSize();
int64_t Size2 = LdSt2->getMemSize();
int64_t PtrDiff;
- if (BasePtr0.BaseReg == BasePtr1.BaseReg) {
-PtrDiff = BasePtr1.Offset - BasePtr0.Offset;
+ if (BasePtr0.getBase() == BasePtr1.getBase() && BasePtr0.hasValidOffset() &&
+ BasePtr1.hasValidOffset()) {
+PtrDiff = BasePtr1.getOffset() - BasePtr0.getOffset();
// If the size of memory access is unknown, do not use it to do analysis.
// One example of unknown
[llvm-branch-commits] [llvm] release/18.x: [GlobalISel] Fix store merging incorrectly classifying an unknown index expr as 0. (#90375) (PR #90673)
https://github.com/aemerson approved this pull request. https://github.com/llvm/llvm-project/pull/90673 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] release/18.x: [GlobalISel] Fix store merging incorrectly classifying an unknown index expr as 0. (#90375) (PR #90673)
llvmbot wrote:
@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-llvm-globalisel
Author: None (llvmbot)
Changes
Backport 19f4d68252b70c81ebb1686a5a31069eda5373de
Requested by: @aemerson
---
Full diff: https://github.com/llvm/llvm-project/pull/90673.diff
4 Files Affected:
- (modified) llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h (+16-4)
- (modified) llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp (+28-20)
- (modified) llvm/test/CodeGen/AArch64/GlobalISel/store-merging.ll (+19)
- (modified) llvm/test/CodeGen/AArch64/GlobalISel/store-merging.mir (+52-10)
``diff
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h
b/llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h
index 0f20a33f3a755c..7990997835d019 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h
@@ -35,11 +35,23 @@ struct LegalityQuery;
class MachineRegisterInfo;
namespace GISelAddressing {
/// Helper struct to store a base, index and offset that forms an address
-struct BaseIndexOffset {
+class BaseIndexOffset {
+private:
Register BaseReg;
Register IndexReg;
- int64_t Offset = 0;
- bool IsIndexSignExt = false;
+ std::optional Offset;
+
+public:
+ BaseIndexOffset() = default;
+ Register getBase() { return BaseReg; }
+ Register getBase() const { return BaseReg; }
+ Register getIndex() { return IndexReg; }
+ Register getIndex() const { return IndexReg; }
+ void setBase(Register NewBase) { BaseReg = NewBase; }
+ void setIndex(Register NewIndex) { IndexReg = NewIndex; }
+ void setOffset(std::optional NewOff) { Offset = NewOff; }
+ bool hasValidOffset() const { return Offset.has_value(); }
+ int64_t getOffset() const { return *Offset; }
};
/// Returns a BaseIndexOffset which describes the pointer in \p Ptr.
@@ -89,7 +101,7 @@ class LoadStoreOpt : public MachineFunctionPass {
// order stores are writing to incremeneting consecutive addresses. So when
// we walk the block in reverse order, the next eligible store must write
to
// an offset one store width lower than CurrentLowestOffset.
-uint64_t CurrentLowestOffset;
+int64_t CurrentLowestOffset;
SmallVector Stores;
// A vector of MachineInstr/unsigned pairs to denote potential aliases that
// need to be checked before the candidate is considered safe to merge. The
diff --git a/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
b/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
index 246aa88b09acf6..ee499c41c558c3 100644
--- a/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
@@ -84,21 +84,20 @@ BaseIndexOffset GISelAddressing::getPointerInfo(Register
Ptr,
MachineRegisterInfo ) {
BaseIndexOffset Info;
Register PtrAddRHS;
- if (!mi_match(Ptr, MRI, m_GPtrAdd(m_Reg(Info.BaseReg), m_Reg(PtrAddRHS {
-Info.BaseReg = Ptr;
-Info.IndexReg = Register();
-Info.IsIndexSignExt = false;
+ Register BaseReg;
+ if (!mi_match(Ptr, MRI, m_GPtrAdd(m_Reg(BaseReg), m_Reg(PtrAddRHS {
+Info.setBase(Ptr);
+Info.setOffset(0);
return Info;
}
-
+ Info.setBase(BaseReg);
auto RHSCst = getIConstantVRegValWithLookThrough(PtrAddRHS, MRI);
if (RHSCst)
-Info.Offset = RHSCst->Value.getSExtValue();
+Info.setOffset(RHSCst->Value.getSExtValue());
// Just recognize a simple case for now. In future we'll need to match
// indexing patterns for base + index + constant.
- Info.IndexReg = PtrAddRHS;
- Info.IsIndexSignExt = false;
+ Info.setIndex(PtrAddRHS);
return Info;
}
@@ -114,15 +113,16 @@ bool GISelAddressing::aliasIsKnownForLoadStore(const
MachineInstr ,
BaseIndexOffset BasePtr0 = getPointerInfo(LdSt1->getPointerReg(), MRI);
BaseIndexOffset BasePtr1 = getPointerInfo(LdSt2->getPointerReg(), MRI);
- if (!BasePtr0.BaseReg.isValid() || !BasePtr1.BaseReg.isValid())
+ if (!BasePtr0.getBase().isValid() || !BasePtr1.getBase().isValid())
return false;
int64_t Size1 = LdSt1->getMemSize();
int64_t Size2 = LdSt2->getMemSize();
int64_t PtrDiff;
- if (BasePtr0.BaseReg == BasePtr1.BaseReg) {
-PtrDiff = BasePtr1.Offset - BasePtr0.Offset;
+ if (BasePtr0.getBase() == BasePtr1.getBase() && BasePtr0.hasValidOffset() &&
+ BasePtr1.hasValidOffset()) {
+PtrDiff = BasePtr1.getOffset() - BasePtr0.getOffset();
// If the size of memory access is unknown, do not use it to do analysis.
// One example of unknown size memory access is to load/store scalable
// vector objects on the stack.
@@ -151,8 +151,8 @@ bool GISelAddressing::aliasIsKnownForLoadStore(const
MachineInstr ,
// able to calculate their relative offset if at least one arises
// from an alloca. However, these allocas cannot overlap and we
// can infer there is no alias.
- auto *Base0Def = getDefIgnoringCopies(BasePtr0.BaseReg, MRI);
- auto *Base1Def = getDefIgnoringCopies(BasePtr1.BaseReg, MRI);
[llvm-branch-commits] [llvm] release/18.x: [GlobalISel] Fix store merging incorrectly classifying an unknown index expr as 0. (#90375) (PR #90673)
llvmbot wrote: @aemerson What do you think about merging this PR to the release branch? https://github.com/llvm/llvm-project/pull/90673 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] release/18.x: [GlobalISel] Fix store merging incorrectly classifying an unknown index expr as 0. (#90375) (PR #90673)
https://github.com/llvmbot created
https://github.com/llvm/llvm-project/pull/90673
Backport 19f4d68252b70c81ebb1686a5a31069eda5373de
Requested by: @aemerson
>From 740c8ec340a1ca74e93b5964bd3a0e5aeb0a4910 Mon Sep 17 00:00:00 2001
From: Amara Emerson
Date: Wed, 1 May 2024 05:42:14 +0800
Subject: [PATCH] [GlobalISel] Fix store merging incorrectly classifying an
unknown index expr as 0. (#90375)
During analysis, we incorrectly leave the offset part of an address info
struct
as zero, when in actual fact we failed to decompose it into base +
offset.
This results in incorrectly assuming that the address is adjacent to
another store
addr. To fix this we wrap the offset in an optional<> so we can
distinguish between
real zero and unknown.
Fixes issue #90242
(cherry picked from commit 19f4d68252b70c81ebb1686a5a31069eda5373de)
---
.../llvm/CodeGen/GlobalISel/LoadStoreOpt.h| 20 --
llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp | 48 --
.../AArch64/GlobalISel/store-merging.ll | 19 ++
.../AArch64/GlobalISel/store-merging.mir | 62 ---
4 files changed, 115 insertions(+), 34 deletions(-)
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h
b/llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h
index 0f20a33f3a755c..7990997835d019 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h
@@ -35,11 +35,23 @@ struct LegalityQuery;
class MachineRegisterInfo;
namespace GISelAddressing {
/// Helper struct to store a base, index and offset that forms an address
-struct BaseIndexOffset {
+class BaseIndexOffset {
+private:
Register BaseReg;
Register IndexReg;
- int64_t Offset = 0;
- bool IsIndexSignExt = false;
+ std::optional Offset;
+
+public:
+ BaseIndexOffset() = default;
+ Register getBase() { return BaseReg; }
+ Register getBase() const { return BaseReg; }
+ Register getIndex() { return IndexReg; }
+ Register getIndex() const { return IndexReg; }
+ void setBase(Register NewBase) { BaseReg = NewBase; }
+ void setIndex(Register NewIndex) { IndexReg = NewIndex; }
+ void setOffset(std::optional NewOff) { Offset = NewOff; }
+ bool hasValidOffset() const { return Offset.has_value(); }
+ int64_t getOffset() const { return *Offset; }
};
/// Returns a BaseIndexOffset which describes the pointer in \p Ptr.
@@ -89,7 +101,7 @@ class LoadStoreOpt : public MachineFunctionPass {
// order stores are writing to incremeneting consecutive addresses. So when
// we walk the block in reverse order, the next eligible store must write
to
// an offset one store width lower than CurrentLowestOffset.
-uint64_t CurrentLowestOffset;
+int64_t CurrentLowestOffset;
SmallVector Stores;
// A vector of MachineInstr/unsigned pairs to denote potential aliases that
// need to be checked before the candidate is considered safe to merge. The
diff --git a/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
b/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
index 246aa88b09acf6..ee499c41c558c3 100644
--- a/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp
@@ -84,21 +84,20 @@ BaseIndexOffset GISelAddressing::getPointerInfo(Register
Ptr,
MachineRegisterInfo ) {
BaseIndexOffset Info;
Register PtrAddRHS;
- if (!mi_match(Ptr, MRI, m_GPtrAdd(m_Reg(Info.BaseReg), m_Reg(PtrAddRHS {
-Info.BaseReg = Ptr;
-Info.IndexReg = Register();
-Info.IsIndexSignExt = false;
+ Register BaseReg;
+ if (!mi_match(Ptr, MRI, m_GPtrAdd(m_Reg(BaseReg), m_Reg(PtrAddRHS {
+Info.setBase(Ptr);
+Info.setOffset(0);
return Info;
}
-
+ Info.setBase(BaseReg);
auto RHSCst = getIConstantVRegValWithLookThrough(PtrAddRHS, MRI);
if (RHSCst)
-Info.Offset = RHSCst->Value.getSExtValue();
+Info.setOffset(RHSCst->Value.getSExtValue());
// Just recognize a simple case for now. In future we'll need to match
// indexing patterns for base + index + constant.
- Info.IndexReg = PtrAddRHS;
- Info.IsIndexSignExt = false;
+ Info.setIndex(PtrAddRHS);
return Info;
}
@@ -114,15 +113,16 @@ bool GISelAddressing::aliasIsKnownForLoadStore(const
MachineInstr ,
BaseIndexOffset BasePtr0 = getPointerInfo(LdSt1->getPointerReg(), MRI);
BaseIndexOffset BasePtr1 = getPointerInfo(LdSt2->getPointerReg(), MRI);
- if (!BasePtr0.BaseReg.isValid() || !BasePtr1.BaseReg.isValid())
+ if (!BasePtr0.getBase().isValid() || !BasePtr1.getBase().isValid())
return false;
int64_t Size1 = LdSt1->getMemSize();
int64_t Size2 = LdSt2->getMemSize();
int64_t PtrDiff;
- if (BasePtr0.BaseReg == BasePtr1.BaseReg) {
-PtrDiff = BasePtr1.Offset - BasePtr0.Offset;
+ if (BasePtr0.getBase() == BasePtr1.getBase() && BasePtr0.hasValidOffset() &&
+ BasePtr1.hasValidOffset()) {
+PtrDiff = BasePtr1.getOffset() - BasePtr0.getOffset();
// If the size of memory
[llvm-branch-commits] [llvm] release/18.x: [GlobalISel] Fix store merging incorrectly classifying an unknown index expr as 0. (#90375) (PR #90673)
https://github.com/llvmbot milestoned https://github.com/llvm/llvm-project/pull/90673 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
