Skip to content

Commit 8ee5759

Browse files
committed
Strip undef implying attributes when moving calls
When hoisting/moving calls to locations, we strip unknown metadata. Such calls are usually marked `speculatable`, i.e. they are guaranteed to not cause undefined behaviour when run anywhere. So, we should strip attributes that can cause immediate undefined behaviour if those attributes are not valid in the context where the call is moved to. This patch introduces such an API and uses it in relevant passes. See updated tests. Fix for PR50744. Reviewed By: nikic, jdoerfert, lebedev.ri Differential Revision: https://reviews.llvm.org/D104641
1 parent d225de6 commit 8ee5759

File tree

8 files changed

+113
-25
lines changed

8 files changed

+113
-25
lines changed

llvm/include/llvm/IR/Instruction.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,8 @@ class Instruction : public User,
332332
/// @{
333333
/// Passes are required to drop metadata they don't understand. This is a
334334
/// convenience method for passes to do so.
335+
/// dropUndefImplyingAttrsAndUnknownMetadata should be used instead of
336+
/// this API if the Instruction being modified is a call.
335337
void dropUnknownNonDebugMetadata(ArrayRef<unsigned> KnownIDs);
336338
void dropUnknownNonDebugMetadata() {
337339
return dropUnknownNonDebugMetadata(None);
@@ -391,6 +393,13 @@ class Instruction : public User,
391393
/// having non-poison inputs.
392394
void dropPoisonGeneratingFlags();
393395

396+
/// This function drops non-debug unknown metadata (through
397+
/// dropUnknownNonDebugMetadata). For calls, it also drops parameter and
398+
/// return attributes that can cause undefined behaviour. Both of these should
399+
/// be done by passes which move instructions in IR.
400+
void
401+
dropUndefImplyingAttrsAndUnknownMetadata(ArrayRef<unsigned> KnownIDs = {});
402+
394403
/// Determine whether the exact flag is set.
395404
bool isExact() const;
396405

llvm/lib/IR/Instruction.cpp

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,23 @@ void Instruction::dropPoisonGeneratingFlags() {
165165
// TODO: FastMathFlags!
166166
}
167167

168+
void Instruction::dropUndefImplyingAttrsAndUnknownMetadata(
169+
ArrayRef<unsigned> KnownIDs) {
170+
dropUnknownNonDebugMetadata(KnownIDs);
171+
auto *CB = dyn_cast<CallBase>(this);
172+
if (!CB)
173+
return;
174+
// For call instructions, we also need to drop parameter and return attributes
175+
// that are can cause UB if the call is moved to a location where the
176+
// attribute is not valid.
177+
AttributeList AL = CB->getAttributes();
178+
if (AL.isEmpty())
179+
return;
180+
AttrBuilder UBImplyingAttributes = AttributeFuncs::getUBImplyingAttributes();
181+
for (unsigned ArgNo = 0; ArgNo < CB->getNumArgOperands(); ArgNo++)
182+
CB->removeParamAttrs(ArgNo, UBImplyingAttributes);
183+
CB->removeAttributes(AttributeList::ReturnIndex, UBImplyingAttributes);
184+
}
168185

169186
bool Instruction::isExact() const {
170187
return cast<PossiblyExactOperator>(this)->isExact();

llvm/lib/Transforms/Scalar/LICM.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1810,12 +1810,16 @@ static void hoist(Instruction &I, const DominatorTree *DT, const Loop *CurLoop,
18101810
// Conservatively strip all metadata on the instruction unless we were
18111811
// guaranteed to execute I if we entered the loop, in which case the metadata
18121812
// is valid in the loop preheader.
1813-
if (I.hasMetadataOtherThanDebugLoc() &&
1813+
// Similarly, If I is a call and it is not guaranteed to execute in the loop,
1814+
// then moving to the preheader means we should strip attributes on the call
1815+
// that can cause UB since we may be hoisting above conditions that allowed
1816+
// inferring those attributes. They may not be valid at the preheader.
1817+
if ((I.hasMetadataOtherThanDebugLoc() || isa<CallInst>(I)) &&
18141818
// The check on hasMetadataOtherThanDebugLoc is to prevent us from burning
18151819
// time in isGuaranteedToExecute if we don't actually have anything to
18161820
// drop. It is a compile time optimization, not required for correctness.
18171821
!SafetyInfo->isGuaranteedToExecute(I, DT, CurLoop))
1818-
I.dropUnknownNonDebugMetadata();
1822+
I.dropUndefImplyingAttrsAndUnknownMetadata();
18191823

18201824
if (isa<PHINode>(I))
18211825
// Move the new node to the end of the phi list in the destination block.

llvm/lib/Transforms/Utils/Local.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2822,7 +2822,7 @@ void llvm::hoistAllInstructionsInto(BasicBlock *DomBlock, Instruction *InsertPt,
28222822

28232823
for (BasicBlock::iterator II = BB->begin(), IE = BB->end(); II != IE;) {
28242824
Instruction *I = &*II;
2825-
I->dropUnknownNonDebugMetadata();
2825+
I->dropUndefImplyingAttrsAndUnknownMetadata();
28262826
if (I->isUsedByMetadata())
28272827
dropDebugUsers(*I);
28282828
if (I->isDebugOrPseudoInst()) {

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1083,7 +1083,10 @@ static void CloneInstructionsIntoPredecessorBlockAndUpdateSSAUses(
10831083
// For an analogous reason, we must also drop all the metadata whose
10841084
// semantics we don't understand. We *can* preserve !annotation, because
10851085
// it is tied to the instruction itself, not the value or position.
1086-
NewBonusInst->dropUnknownNonDebugMetadata(LLVMContext::MD_annotation);
1086+
// Similarly strip attributes on call parameters that may cause UB in
1087+
// location the call is moved to.
1088+
NewBonusInst->dropUndefImplyingAttrsAndUnknownMetadata(
1089+
LLVMContext::MD_annotation);
10871090

10881091
PredBlock->getInstList().insert(PTI->getIterator(), NewBonusInst);
10891092
NewBonusInst->takeName(&BonusInst);
@@ -2492,10 +2495,12 @@ bool SimplifyCFGOpt::SpeculativelyExecuteBB(BranchInst *BI, BasicBlock *ThenBB,
24922495
// Conservatively strip all metadata on the instruction. Drop the debug loc
24932496
// to avoid making it appear as if the condition is a constant, which would
24942497
// be misleading while debugging.
2498+
// Similarly strip attributes that maybe dependent on condition we are
2499+
// hoisting above.
24952500
for (auto &I : *ThenBB) {
24962501
if (!SpeculatedStoreValue || &I != SpeculatedStore)
24972502
I.setDebugLoc(DebugLoc());
2498-
I.dropUnknownNonDebugMetadata();
2503+
I.dropUndefImplyingAttrsAndUnknownMetadata();
24992504
}
25002505

25012506
// Hoist the instructions.

llvm/test/Transforms/LICM/call-hoisting.ll

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -23,16 +23,22 @@ exit:
2323
ret void
2424
}
2525

26-
declare i32 @spec(i32* %p) readonly argmemonly nounwind speculatable
26+
declare i32 @spec(i32* %p, i32* %q) readonly argmemonly nounwind speculatable
2727

28-
; FIXME: We should strip the nonnull callsite attribute on spec call's argument since it is
29-
; may not be valid when hoisted to preheader.
28+
; We should strip the dereferenceable callsite attribute on spec call's argument since it is
29+
; can cause UB in the speculatable call when hoisted to preheader.
30+
; However, we need not strip the nonnull attribute since it just propagates
31+
; poison if the parameter was indeed null.
3032
define void @test_strip_attribute(i32* noalias %loc, i32* noalias %sink, i32* %q) {
31-
; CHECK-LABEL: test_strip_attribute
32-
; CHECK-LABEL: entry
33-
; CHECK-NEXT: %ret = call i32 @load(i32* %loc)
34-
; CHECK-NEXT: %nullchk = icmp eq i32* %q, null
35-
; CHECK-NEXT: %ret2 = call i32 @spec(i32* nonnull %q)
33+
; CHECK-LABEL: @test_strip_attribute(
34+
; CHECK-NEXT: entry:
35+
; CHECK-NEXT: [[RET:%.*]] = call i32 @load(i32* [[LOC:%.*]])
36+
; CHECK-NEXT: [[NULLCHK:%.*]] = icmp eq i32* [[Q:%.*]], null
37+
; CHECK-NEXT: [[RET2:%.*]] = call i32 @spec(i32* nonnull [[Q]], i32* [[LOC]])
38+
; CHECK-NEXT: br label [[LOOP:%.*]]
39+
; CHECK: loop:
40+
; CHECK-NEXT: [[IV:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[IV_NEXT:%.*]], [[ISNULL:%.*]] ]
41+
; CHECK-NEXT: br i1 [[NULLCHK]], label [[ISNULL]], label [[NONNULLBB:%.*]]
3642
entry:
3743
br label %loop
3844

@@ -42,11 +48,11 @@ loop:
4248
%nullchk = icmp eq i32* %q, null
4349
br i1 %nullchk, label %isnull, label %nonnullbb
4450

45-
nonnullbb:
46-
%ret2 = call i32 @spec(i32* nonnull %q)
51+
nonnullbb:
52+
%ret2 = call i32 @spec(i32* nonnull %q, i32* dereferenceable(12) %loc)
4753
br label %isnull
4854

49-
isnull:
55+
isnull:
5056
store volatile i32 %ret, i32* %sink
5157
%iv.next = add i32 %iv, 1
5258
%cmp = icmp slt i32 %iv, 200
@@ -90,7 +96,7 @@ loop:
9096
call void @store(i32 0, i32* %loc)
9197
%iv.next = add i32 %iv, 1
9298
br i1 %earlycnd, label %exit1, label %backedge
93-
99+
94100
backedge:
95101
%cmp = icmp slt i32 %iv, 200
96102
br i1 %cmp, label %loop, label %exit2
@@ -174,7 +180,7 @@ loop:
174180
%v = load i32, i32* %loc
175181
%earlycnd = icmp eq i32 %v, 198
176182
br i1 %earlycnd, label %exit1, label %backedge
177-
183+
178184
backedge:
179185
%iv.next = add i32 %iv, 1
180186
%cmp = icmp slt i32 %iv, 200

llvm/test/Transforms/SimplifyCFG/fold-branch-to-common-dest.ll

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,9 @@ final_right:
118118
ret void
119119
}
120120

121-
; FIXME: When we fold the dispatch block into pred, the call is moved to pred
122-
; and the attribute nonnull is no longer valid on paramater. We should drop it.
121+
; When we fold the dispatch block into pred, the call is moved to pred
122+
; and the attribute nonnull propagates poison paramater. However, since the
123+
; function is speculatable, it can never cause UB. So, we need not technically drop it.
123124
define void @one_pred_with_spec_call(i8 %v0, i8 %v1, i32* %p) {
124125
; CHECK-LABEL: @one_pred_with_spec_call(
125126
; CHECK-NEXT: pred:
@@ -133,7 +134,6 @@ define void @one_pred_with_spec_call(i8 %v0, i8 %v1, i32* %p) {
133134
; CHECK: final_right:
134135
; CHECK-NEXT: call void @sideeffect0()
135136
; CHECK-NEXT: br label [[COMMON_RET]]
136-
;
137137
pred:
138138
%c0 = icmp ne i32* %p, null
139139
br i1 %c0, label %dispatch, label %final_right
@@ -151,6 +151,29 @@ final_right:
151151
ret void
152152
}
153153

154+
; Drop dereferenceable on the parameter
155+
define void @one_pred_with_spec_call_deref(i8 %v0, i8 %v1, i32* %p) {
156+
; CHECK-LABEL: one_pred_with_spec_call_deref
157+
; CHECK-LABEL: pred:
158+
; CHECK: %c0 = icmp ne i32* %p, null
159+
; CHECK: %x = call i32 @speculate_call(i32* %p)
160+
pred:
161+
%c0 = icmp ne i32* %p, null
162+
br i1 %c0, label %dispatch, label %final_right
163+
164+
dispatch:
165+
%x = call i32 @speculate_call(i32* dereferenceable(12) %p)
166+
%c1 = icmp eq i8 %v1, 0
167+
br i1 %c1, label %final_left, label %final_right
168+
169+
final_left:
170+
ret void
171+
172+
final_right:
173+
call void @sideeffect0()
174+
ret void
175+
}
176+
154177
define void @two_preds_with_extra_op(i8 %v0, i8 %v1, i8 %v2, i8 %v3) {
155178
; CHECK-LABEL: @two_preds_with_extra_op(
156179
; CHECK-NEXT: entry:

llvm/test/Transforms/SimplifyCFG/speculate-call.ll

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,28 +20,52 @@ define i32 @func() #0 {
2020
ret i32 1
2121
}
2222

23-
; FIXME: We should correctly drop the attribute since it may no longer be valid
23+
; We should correctly drop the attribute since it may no longer be valid
2424
; in the context the call is moved to.
25+
; Since the function is speculatable, the nonnull attribute need not be dropped
26+
; since it propagates poison (and call executes fine) if the parameter is indeed
27+
; null.
2528
define i32 @strip_attr(i32 * %p) {
2629
; CHECK-LABEL: strip_attr
2730
; CHECK-LABEL: entry:
2831
; CHECK: %nullchk = icmp ne i32* %p, null
29-
; CHECK: %val = call i32 @func_deref(i32* nonnull %p)
32+
; CHECK: %val = call i32 @func_nonnull(i32* nonnull %p)
3033
; CHECK: select
3134
entry:
3235
%nullchk = icmp ne i32* %p, null
3336
br i1 %nullchk, label %if, label %end
3437

3538
if:
36-
%val = call i32 @func_deref(i32* nonnull %p) #1
39+
%val = call i32 @func_nonnull(i32* nonnull %p) #1
3740
br label %end
3841

3942
end:
4043
%ret = phi i32 [%val, %if], [0, %entry]
4144
ret i32 %ret
4245
}
4346

44-
declare i32 @func_deref(i32*) #1
47+
; We should strip the deref attribute since it can cause UB when the
48+
; speculatable call is moved.
49+
define i32 @strip_attr2(i32 * %p) {
50+
; CHECK-LABEL: strip_attr2
51+
; CHECK-LABEL: entry:
52+
; CHECK: %nullchk = icmp ne i32* %p, null
53+
; CHECK: %val = call i32 @func_nonnull(i32* %p)
54+
; CHECK: select
55+
entry:
56+
%nullchk = icmp ne i32* %p, null
57+
br i1 %nullchk, label %if, label %end
58+
59+
if:
60+
%val = call i32 @func_nonnull(i32* dereferenceable(12) %p) #1
61+
br label %end
62+
63+
end:
64+
%ret = phi i32 [%val, %if], [0, %entry]
65+
ret i32 %ret
66+
}
67+
68+
declare i32 @func_nonnull(i32*) #1
4569

4670
attributes #0 = { nounwind readnone speculatable }
4771
attributes #1 = { nounwind argmemonly speculatable }

0 commit comments

Comments
 (0)