[llvm] Reland "[Support] Assert that DomTree nodes share parent"" (PR #102782)
Alexis Engelke via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 13 00:51:05 PDT 2024
https://github.com/aengelke updated https://github.com/llvm/llvm-project/pull/102782
>From 2218df4620584e1748cde41e7bc0fd6cd88da58e Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at gmail.com>
Date: Sat, 10 Aug 2024 18:36:32 -0700
Subject: [PATCH 1/4] Revert "Revert "[Support] Assert that DomTree nodes share
parent" (#102780)"
This reverts commit 3c3df1bef84bd509bdd2b6033bc9bb3653826388.
---
llvm/include/llvm/Support/GenericDomTree.h | 2 ++
llvm/lib/Analysis/TypeMetadataUtils.cpp | 2 ++
.../Scalar/AlignmentFromAssumptions.cpp | 1 +
llvm/lib/Transforms/Scalar/LoopFuse.cpp | 8 +++--
.../AlignmentFromAssumptions/domtree-crash.ll | 33 +++++++++++++++++++
5 files changed, 44 insertions(+), 2 deletions(-)
create mode 100644 llvm/test/Transforms/AlignmentFromAssumptions/domtree-crash.ll
diff --git a/llvm/include/llvm/Support/GenericDomTree.h b/llvm/include/llvm/Support/GenericDomTree.h
index 7e2b68e6faea29..45ef38b965b752 100644
--- a/llvm/include/llvm/Support/GenericDomTree.h
+++ b/llvm/include/llvm/Support/GenericDomTree.h
@@ -397,6 +397,8 @@ class DominatorTreeBase {
/// may (but is not required to) be null for a forward (backwards)
/// statically unreachable block.
DomTreeNodeBase<NodeT> *getNode(const NodeT *BB) const {
+ assert((!BB || Parent == NodeTrait::getParent(const_cast<NodeT *>(BB))) &&
+ "cannot get DomTreeNode of block with different parent");
if (auto Idx = getNodeIndex(BB); Idx && *Idx < DomTreeNodes.size())
return DomTreeNodes[*Idx].get();
return nullptr;
diff --git a/llvm/lib/Analysis/TypeMetadataUtils.cpp b/llvm/lib/Analysis/TypeMetadataUtils.cpp
index 67ce1540112bb7..9ec0785eb5034d 100644
--- a/llvm/lib/Analysis/TypeMetadataUtils.cpp
+++ b/llvm/lib/Analysis/TypeMetadataUtils.cpp
@@ -33,6 +33,8 @@ findCallsAtConstantOffset(SmallVectorImpl<DevirtCallSite> &DevirtCalls,
// after indirect call promotion and inlining, where we may have uses
// of the vtable pointer guarded by a function pointer check, and a fallback
// indirect call.
+ if (CI->getFunction() != User->getFunction())
+ continue;
if (!DT.dominates(CI, User))
continue;
if (isa<BitCastInst>(User)) {
diff --git a/llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp b/llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp
index f3422a705dca7a..8555ef5c22f827 100644
--- a/llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp
+++ b/llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp
@@ -208,6 +208,7 @@ bool AlignmentFromAssumptionsPass::processAssumption(CallInst *ACall,
continue;
if (Instruction *K = dyn_cast<Instruction>(J))
+ if (K->getFunction() == ACall->getFunction())
WorkList.push_back(K);
}
diff --git a/llvm/lib/Transforms/Scalar/LoopFuse.cpp b/llvm/lib/Transforms/Scalar/LoopFuse.cpp
index 8512b2accbe7c2..fe0e30d1965e05 100644
--- a/llvm/lib/Transforms/Scalar/LoopFuse.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopFuse.cpp
@@ -1729,7 +1729,9 @@ struct LoopFuser {
// mergeLatch may remove the only block in FC1.
SE.forgetLoop(FC1.L);
SE.forgetLoop(FC0.L);
- SE.forgetLoopDispositions();
+ // Forget block dispositions as well, so that there are no dangling
+ // pointers to erased/free'ed blocks.
+ SE.forgetBlockAndLoopDispositions();
// Move instructions from FC0.Latch to FC1.Latch.
// Note: mergeLatch requires an updated DT.
@@ -2023,7 +2025,9 @@ struct LoopFuser {
// mergeLatch may remove the only block in FC1.
SE.forgetLoop(FC1.L);
SE.forgetLoop(FC0.L);
- SE.forgetLoopDispositions();
+ // Forget block dispositions as well, so that there are no dangling
+ // pointers to erased/free'ed blocks.
+ SE.forgetBlockAndLoopDispositions();
// Move instructions from FC0.Latch to FC1.Latch.
// Note: mergeLatch requires an updated DT.
diff --git a/llvm/test/Transforms/AlignmentFromAssumptions/domtree-crash.ll b/llvm/test/Transforms/AlignmentFromAssumptions/domtree-crash.ll
new file mode 100644
index 00000000000000..c7fc1dc6996718
--- /dev/null
+++ b/llvm/test/Transforms/AlignmentFromAssumptions/domtree-crash.ll
@@ -0,0 +1,33 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -passes=alignment-from-assumptions -S < %s | FileCheck %s
+
+; The alignment assumption is a global, which has users in a different
+; function. Test that in this case the dominator tree is only queried with
+; blocks from the same function.
+
+ at global = external constant [192 x i8]
+
+define void @fn1() {
+; CHECK-LABEL: define void @fn1() {
+; CHECK-NEXT: call void @llvm.assume(i1 false) [ "align"(ptr @global, i64 1) ]
+; CHECK-NEXT: ret void
+;
+ call void @llvm.assume(i1 false) [ "align"(ptr @global, i64 1) ]
+ ret void
+}
+
+define void @fn2() {
+; CHECK-LABEL: define void @fn2() {
+; CHECK-NEXT: ret void
+; CHECK: [[LOOP:.*]]:
+; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds i8, ptr @global, i64 0
+; CHECK-NEXT: [[LOAD:%.*]] = load i64, ptr [[GEP]], align 1
+; CHECK-NEXT: br label %[[LOOP]]
+;
+ ret void
+
+loop:
+ %gep = getelementptr inbounds i8, ptr @global, i64 0
+ %load = load i64, ptr %gep, align 1
+ br label %loop
+}
>From 1739628f12950e3ddbd80418750b93cdc11b48e8 Mon Sep 17 00:00:00 2001
From: Alexis Engelke <engelke at in.tum.de>
Date: Sun, 11 Aug 2024 05:55:56 +0000
Subject: [PATCH 2/4] Fix SLPVectorize assumption that all users are in the
same function
---
.../Transforms/Vectorize/SLPVectorizer.cpp | 4 +-
.../const-in-different-functions.ll | 46 +++++++++++++++++++
2 files changed, 49 insertions(+), 1 deletion(-)
create mode 100644 llvm/test/Transforms/SLPVectorizer/const-in-different-functions.ll
diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index 186b382addd710..91e180f9eea13c 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -5998,7 +5998,9 @@ BoUpSLP::collectUserStores(const BoUpSLP::TreeEntry *TE) const {
// Collect stores per pointer object.
for (User *U : V->users()) {
auto *SI = dyn_cast<StoreInst>(U);
- if (SI == nullptr || !SI->isSimple() ||
+ // Test whether we can handle the store. If V is a constant, its users
+ // might be in different functions.
+ if (SI == nullptr || !SI->isSimple() || SI->getFunction() != F ||
!isValidElementType(SI->getValueOperand()->getType()))
continue;
// Skip entry if already
diff --git a/llvm/test/Transforms/SLPVectorizer/const-in-different-functions.ll b/llvm/test/Transforms/SLPVectorizer/const-in-different-functions.ll
new file mode 100644
index 00000000000000..29a8f15733c450
--- /dev/null
+++ b/llvm/test/Transforms/SLPVectorizer/const-in-different-functions.ll
@@ -0,0 +1,46 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -mtriple=x86_64 -passes=slp-vectorizer < %s | FileCheck %s
+
+; Test that SLP vectorize doesn't crash if a stored constant is used in multiple
+; functions.
+
+define void @_Z1hPfl() {
+; CHECK-LABEL: define void @_Z1hPfl() {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: [[TMP0:%.*]] = getelementptr i8, ptr null, i64 28
+; CHECK-NEXT: store <2 x float> <float 0.000000e+00, float 1.000000e+00>, ptr [[TMP0]], align 4
+; CHECK-NEXT: ret void
+;
+entry:
+ %0 = getelementptr i8, ptr null, i64 28
+ store float 0.000000e+00, ptr %0, align 4
+ %1 = getelementptr i8, ptr null, i64 32
+ store float 1.000000e+00, ptr %1, align 16
+ ret void
+}
+
+define void @_Z1mv(i64 %arrayidx4.i.2.idx) {
+; CHECK-LABEL: define void @_Z1mv(
+; CHECK-SAME: i64 [[ARRAYIDX4_I_2_IDX:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: ret void
+; CHECK: [[FOR_COND1_PREHEADER_LR_PH_I:.*:]]
+; CHECK-NEXT: br label %[[FOR_COND1_PREHEADER_I:.*]]
+; CHECK: [[FOR_COND1_PREHEADER_I]]:
+; CHECK-NEXT: store float 1.000000e+00, ptr null, align 4
+; CHECK-NEXT: [[ARRAYIDX4_I_2:%.*]] = getelementptr i8, ptr null, i64 [[ARRAYIDX4_I_2_IDX]]
+; CHECK-NEXT: store float 0.000000e+00, ptr [[ARRAYIDX4_I_2]], align 4
+; CHECK-NEXT: br label %[[FOR_COND1_PREHEADER_I]]
+;
+entry:
+ ret void
+
+for.cond1.preheader.lr.ph.i: ; No predecessors!
+ br label %for.cond1.preheader.i
+
+for.cond1.preheader.i: ; preds = %for.cond1.preheader.i, %for.cond1.preheader.lr.ph.i
+ store float 1.000000e+00, ptr null, align 4
+ %arrayidx4.i.2 = getelementptr i8, ptr null, i64 %arrayidx4.i.2.idx
+ store float 0.000000e+00, ptr %arrayidx4.i.2, align 4
+ br label %for.cond1.preheader.i
+}
>From 084d02577eb68dd2b6260b9b1d12a61631e8d799 Mon Sep 17 00:00:00 2001
From: Alexis Engelke <engelke at in.tum.de>
Date: Sun, 11 Aug 2024 07:39:53 +0000
Subject: [PATCH 3/4] Address comments
---
.../{ => X86}/const-in-different-functions.ll | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
rename llvm/test/Transforms/SLPVectorizer/{ => X86}/const-in-different-functions.ll (75%)
diff --git a/llvm/test/Transforms/SLPVectorizer/const-in-different-functions.ll b/llvm/test/Transforms/SLPVectorizer/X86/const-in-different-functions.ll
similarity index 75%
rename from llvm/test/Transforms/SLPVectorizer/const-in-different-functions.ll
rename to llvm/test/Transforms/SLPVectorizer/X86/const-in-different-functions.ll
index 29a8f15733c450..2e473f4f2c213c 100644
--- a/llvm/test/Transforms/SLPVectorizer/const-in-different-functions.ll
+++ b/llvm/test/Transforms/SLPVectorizer/X86/const-in-different-functions.ll
@@ -4,17 +4,19 @@
; Test that SLP vectorize doesn't crash if a stored constant is used in multiple
; functions.
+ at p = external global [64 x float]
+
define void @_Z1hPfl() {
; CHECK-LABEL: define void @_Z1hPfl() {
; CHECK-NEXT: [[ENTRY:.*:]]
-; CHECK-NEXT: [[TMP0:%.*]] = getelementptr i8, ptr null, i64 28
+; CHECK-NEXT: [[TMP0:%.*]] = getelementptr i8, ptr @p, i64 28
; CHECK-NEXT: store <2 x float> <float 0.000000e+00, float 1.000000e+00>, ptr [[TMP0]], align 4
; CHECK-NEXT: ret void
;
entry:
- %0 = getelementptr i8, ptr null, i64 28
+ %0 = getelementptr i8, ptr @p, i64 28
store float 0.000000e+00, ptr %0, align 4
- %1 = getelementptr i8, ptr null, i64 32
+ %1 = getelementptr i8, ptr @p, i64 32
store float 1.000000e+00, ptr %1, align 16
ret void
}
@@ -27,8 +29,8 @@ define void @_Z1mv(i64 %arrayidx4.i.2.idx) {
; CHECK: [[FOR_COND1_PREHEADER_LR_PH_I:.*:]]
; CHECK-NEXT: br label %[[FOR_COND1_PREHEADER_I:.*]]
; CHECK: [[FOR_COND1_PREHEADER_I]]:
-; CHECK-NEXT: store float 1.000000e+00, ptr null, align 4
-; CHECK-NEXT: [[ARRAYIDX4_I_2:%.*]] = getelementptr i8, ptr null, i64 [[ARRAYIDX4_I_2_IDX]]
+; CHECK-NEXT: store float 1.000000e+00, ptr @p, align 4
+; CHECK-NEXT: [[ARRAYIDX4_I_2:%.*]] = getelementptr i8, ptr @p, i64 [[ARRAYIDX4_I_2_IDX]]
; CHECK-NEXT: store float 0.000000e+00, ptr [[ARRAYIDX4_I_2]], align 4
; CHECK-NEXT: br label %[[FOR_COND1_PREHEADER_I]]
;
@@ -39,8 +41,8 @@ for.cond1.preheader.lr.ph.i: ; No predecessors!
br label %for.cond1.preheader.i
for.cond1.preheader.i: ; preds = %for.cond1.preheader.i, %for.cond1.preheader.lr.ph.i
- store float 1.000000e+00, ptr null, align 4
- %arrayidx4.i.2 = getelementptr i8, ptr null, i64 %arrayidx4.i.2.idx
+ store float 1.000000e+00, ptr @p, align 4
+ %arrayidx4.i.2 = getelementptr i8, ptr @p, i64 %arrayidx4.i.2.idx
store float 0.000000e+00, ptr %arrayidx4.i.2, align 4
br label %for.cond1.preheader.i
}
>From d0b1a582fd33e8c3605c027883c6deb35757f560 Mon Sep 17 00:00:00 2001
From: Alexis Engelke <engelke at in.tum.de>
Date: Tue, 13 Aug 2024 07:50:05 +0000
Subject: [PATCH 4/4] Address comments
---
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index 91e180f9eea13c..edacb2fb33540f 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -5991,6 +5991,9 @@ BoUpSLP::collectUserStores(const BoUpSLP::TreeEntry *TE) const {
DenseMap<Value *, SmallVector<StoreInst *>> PtrToStoresMap;
for (unsigned Lane : seq<unsigned>(0, TE->Scalars.size())) {
Value *V = TE->Scalars[Lane];
+ // Don't iterate over the users of constant data.
+ if (isa<ConstantData>(V))
+ continue;
// To save compilation time we don't visit if we have too many users.
if (V->hasNUsesOrMore(UsesLimit))
break;
@@ -5998,8 +6001,8 @@ BoUpSLP::collectUserStores(const BoUpSLP::TreeEntry *TE) const {
// Collect stores per pointer object.
for (User *U : V->users()) {
auto *SI = dyn_cast<StoreInst>(U);
- // Test whether we can handle the store. If V is a constant, its users
- // might be in different functions.
+ // Test whether we can handle the store. V might be a global, which could
+ // be used in a different function.
if (SI == nullptr || !SI->isSimple() || SI->getFunction() != F ||
!isValidElementType(SI->getValueOperand()->getType()))
continue;
More information about the llvm-commits
mailing list