Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reland "[Support] Assert that DomTree nodes share parent"" #102782

Merged
merged 4 commits into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions llvm/include/llvm/Support/GenericDomTree.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions llvm/lib/Analysis/TypeMetadataUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
8 changes: 6 additions & 2 deletions llvm/lib/Transforms/Scalar/LoopFuse.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 3 additions & 1 deletion llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
33 changes: 33 additions & 0 deletions llvm/test/Transforms/AlignmentFromAssumptions/domtree-crash.ll
Original file line number Diff line number Diff line change
@@ -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.

@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
}
46 changes: 46 additions & 0 deletions llvm/test/Transforms/SLPVectorizer/const-in-different-functions.ll
Original file line number Diff line number Diff line change
@@ -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
aengelke marked this conversation as resolved.
Show resolved Hide resolved

; 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you replace null with a global here to make the code not be immediate UB?

Is the code iterating over null users or over 0.000000e+00/1.000000e+00 users? If it's the latter, than this code probably needs a larger fix, because iterating over ConstantData users is not allowed in general. (Also applies to null, but in that case a global is a plausible replacement.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's iterating over the ConstantData users. I'm not familiar with SLP, please advise how to fix the larger problem. (If iterating over such users is not allowed, why is there no assertion for this?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's iterating over the ConstantData users. I'm not familiar with SLP, please advise how to fix the larger problem.

Adding an isa<ConstantData>(V) break; in this function should work. (You should still keep your check, in case we're iterating over GV users.)

If iterating over such users is not allowed, why is there no assertion for this?

Nobody has implemented it :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the extra check.

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
}
Loading