Reapply "[JumpThreading] Freeze undef/poison select condition in unfoldSelectInstr"#199736
Open
antoniofrighetto wants to merge 1 commit into
Open
Reapply "[JumpThreading] Freeze undef/poison select condition in unfoldSelectInstr"#199736antoniofrighetto wants to merge 1 commit into
antoniofrighetto wants to merge 1 commit into
Conversation
…ldSelectInstr" When turning a select into a branch, make sure we freeze the condition when it may be poison or undef. Fixes: llvm#199702 Co-authored-by: Justin Lebar <justin.lebar@gmail.com>
|
@llvm/pr-subscribers-llvm-transforms Author: Antonio Frighetto (antoniofrighetto) ChangesWhen turning a select into a branch, make sure we freeze the condition when it may be poison or undef. Original patch: #199408. Fixes: #199702. Co-authored-by: Justin Lebar <justin.lebar@gmail.com> Full diff: https://github.com/llvm/llvm-project/pull/199736.diff 4 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/JumpThreading.cpp b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
index 415136b612ac2..bc0d8454492dc 100644
--- a/llvm/lib/Transforms/Scalar/JumpThreading.cpp
+++ b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
@@ -2785,11 +2785,19 @@ void JumpThreadingPass::unfoldSelectInstr(BasicBlock *Pred, BasicBlock *BB,
UncondBrInst *PredTerm = cast<UncondBrInst>(Pred->getTerminator());
BasicBlock *NewBB = BasicBlock::Create(BB->getContext(), "select.unfold",
BB->getParent(), BB);
+ // When converting a select into a branch, make sure we freeze the condition
+ // when it may be poison or undef.
+ Value *Cond = SI->getCondition();
+ if (!isGuaranteedNotToBeUndefOrPoison(Cond)) {
+ Cond = new FreezeInst(Cond, "cond.fr", SI->getIterator());
+ cast<Instruction>(Cond)->setDebugLoc(DebugLoc::getTemporary());
+ }
+
// Move the unconditional branch to NewBB.
PredTerm->removeFromParent();
PredTerm->insertInto(NewBB, NewBB->end());
// Create a conditional branch and update PHI nodes.
- auto *BI = CondBrInst::Create(SI->getCondition(), NewBB, BB, Pred);
+ auto *BI = CondBrInst::Create(Cond, NewBB, BB, Pred);
BI->applyMergedLocation(PredTerm->getDebugLoc(), SI->getDebugLoc());
BI->copyMetadata(*SI, {LLVMContext::MD_prof});
SIUse->setIncomingValue(Idx, SI->getFalseValue());
diff --git a/llvm/test/Transforms/JumpThreading/branch-debug-info2.ll b/llvm/test/Transforms/JumpThreading/branch-debug-info2.ll
index 59b6ef51d670f..021b3c0290170 100644
--- a/llvm/test/Transforms/JumpThreading/branch-debug-info2.ll
+++ b/llvm/test/Transforms/JumpThreading/branch-debug-info2.ll
@@ -1,5 +1,31 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
; RUN: opt -passes=jump-threading -S %s -o - | FileCheck %s
define void @prepare_next_shadow() !dbg !7 {
+; CHECK-LABEL: define void @prepare_next_shadow(
+; CHECK-SAME: ) !dbg [[DBG7:![0-9]+]] {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: br label %[[SHADOW_TO_PTR_EXIT:.*]], !dbg [[DBG9:![0-9]+]]
+; CHECK: [[SHADOW_TO_PTR_EXIT]]:
+; CHECK-NEXT: [[SHR_I:%.*]] = lshr i64 0, 12, !dbg [[DBG10:![0-9]+]]
+; CHECK-NEXT: [[CMP_I:%.*]] = icmp ult i64 [[SHR_I]], undef, !dbg [[DBG11:![0-9]+]]
+; CHECK-NEXT: br i1 [[CMP_I]], label %[[FOR_INC:.*]], label %[[IF_END_I60:.*]], !dbg [[DBG12:![0-9]+]]
+; CHECK: [[IF_END_I60]]:
+; CHECK-NEXT: [[SUB_I:%.*]] = sub i64 [[SHR_I]], undef, !dbg [[DBG13:![0-9]+]]
+; CHECK-NEXT: [[CMP3_I:%.*]] = icmp ugt i64 [[SUB_I]], 32763, !dbg [[DBG14:![0-9]+]]
+; CHECK-NEXT: [[CONV7_I:%.*]] = trunc i64 [[SUB_I]] to i32, !dbg [[DBG15:![0-9]+]]
+; CHECK-NEXT: [[COND_FR:%.*]] = freeze i1 [[CMP3_I]]
+; Jump threading folds the select below into this branch. Ensure debug info is
+; not lost, and is merged from the select and the branch.
+; CHECK-NEXT: br i1 [[COND_FR]], label %[[FOR_INC]], label %[[PTR_TO_SHADOW_EXIT:.*]], !dbg [[DBG16:![0-9]+]]
+; CHECK: [[PTR_TO_SHADOW_EXIT]]:
+; CHECK-NEXT: [[CALL1861:%.*]] = phi i32 [ [[CONV7_I]], %[[IF_END_I60]] ], !dbg [[DBG17:![0-9]+]]
+; CHECK-NEXT: [[CMP19:%.*]] = icmp slt i32 [[CALL1861]], 0, !dbg [[DBG18:![0-9]+]]
+; CHECK-NEXT: br i1 [[CMP19]], label %[[FOR_INC]], label %[[IF_END22:.*]], !dbg [[DBG19:![0-9]+]]
+; CHECK: [[IF_END22]]:
+; CHECK-NEXT: unreachable, !dbg [[DBG20:![0-9]+]]
+; CHECK: [[FOR_INC]]:
+; CHECK-NEXT: br label %[[SHADOW_TO_PTR_EXIT]], !dbg [[DBG21:![0-9]+]]
+;
entry:
br label %for.cond, !dbg !9
@@ -16,11 +42,6 @@ if.end.i60: ; preds = %shadow_to_ptr.exit
%cmp3.i = icmp ugt i64 %sub.i, 32763, !dbg !15
%conv7.i = trunc i64 %sub.i to i32, !dbg !16
%spec.select.i = select i1 %cmp3.i, i32 -1, i32 %conv7.i, !dbg !17
-; Jump threading is going to fold the select in to the branch. Ensure debug
-; info is not lost, and is merged from the select and the branch.
-; CHECK-NOT: br i1 %cmp3.i, label %for.inc, label %if.end22
-; CHECK: br i1 %cmp3.i, label %for.inc, label %if.end22, !dbg [[DBG:![0-9]+]]
-; CHECK: [[DBG]] = !DILocation(line: 9, column: 1, scope: !{{.*}})
br label %ptr_to_shadow.exit, !dbg !17
@@ -68,3 +89,23 @@ for.inc: ; preds = %ptr_to_shadow.exit
!22 = !DILocation(line: 14, column: 1, scope: !7)
!23 = !DILocation(line: 15, column: 1, scope: !7)
!24 = !DILocation(line: 16, column: 1, scope: !7)
+;.
+; CHECK: [[META1:![0-9]+]] = distinct !DICompileUnit(language: DW_LANG_C, file: [[META2:![0-9]+]], producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: [[META3:![0-9]+]])
+; CHECK: [[META2]] = !DIFile(filename: "{{.*}}jump_threading.ll", directory: {{.*}})
+; CHECK: [[META3]] = !{}
+; CHECK: [[DBG7]] = distinct !DISubprogram(name: "prepare_next_shadow", linkageName: "prepare_next_shadow", scope: null, file: [[META2]], line: 1, type: [[META8:![0-9]+]], scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: [[META1]], retainedNodes: [[META3]])
+; CHECK: [[META8]] = !DISubroutineType(types: [[META3]])
+; CHECK: [[DBG9]] = !DILocation(line: 1, column: 1, scope: [[DBG7]])
+; CHECK: [[DBG10]] = !DILocation(line: 3, column: 1, scope: [[DBG7]])
+; CHECK: [[DBG11]] = !DILocation(line: 4, column: 1, scope: [[DBG7]])
+; CHECK: [[DBG12]] = !DILocation(line: 5, column: 1, scope: [[DBG7]])
+; CHECK: [[DBG13]] = !DILocation(line: 6, column: 1, scope: [[DBG7]])
+; CHECK: [[DBG14]] = !DILocation(line: 7, column: 1, scope: [[DBG7]])
+; CHECK: [[DBG15]] = !DILocation(line: 8, column: 1, scope: [[DBG7]])
+; CHECK: [[DBG16]] = !DILocation(line: 9, column: 1, scope: [[DBG7]])
+; CHECK: [[DBG17]] = !DILocation(line: 12, column: 1, scope: [[DBG7]])
+; CHECK: [[DBG18]] = !DILocation(line: 13, column: 1, scope: [[DBG7]])
+; CHECK: [[DBG19]] = !DILocation(line: 14, column: 1, scope: [[DBG7]])
+; CHECK: [[DBG20]] = !DILocation(line: 15, column: 1, scope: [[DBG7]])
+; CHECK: [[DBG21]] = !DILocation(line: 16, column: 1, scope: [[DBG7]])
+;.
diff --git a/llvm/test/Transforms/JumpThreading/select.ll b/llvm/test/Transforms/JumpThreading/select.ll
index 4ec55a66bb8ac..8e6ae8b6d1ee2 100644
--- a/llvm/test/Transforms/JumpThreading/select.ll
+++ b/llvm/test/Transforms/JumpThreading/select.ll
@@ -286,7 +286,8 @@ define void @unfold1(double %x, double %y) nounwind !prof !1 {
; CHECK: cond.false:
; CHECK-NEXT: [[ADD:%.*]] = fadd double [[X]], [[Y]]
; CHECK-NEXT: [[CMP1:%.*]] = fcmp ogt double [[ADD]], 1.000000e+01
-; CHECK-NEXT: br i1 [[CMP1]], label [[COND_END4]], label [[IF_THEN:%.*]], !prof [[PROF1:![0-9]+]]
+; CHECK-NEXT: [[COND_FR:%.*]] = freeze i1 [[CMP1]]
+; CHECK-NEXT: br i1 [[COND_FR]], label [[COND_END4]], label [[IF_THEN:%.*]], !prof [[PROF1:![0-9]+]]
; CHECK: cond.end4:
; CHECK-NEXT: [[COND5:%.*]] = phi double [ [[SUB]], [[ENTRY:%.*]] ], [ [[ADD]], [[COND_FALSE]] ]
; CHECK-NEXT: [[CMP6:%.*]] = fcmp oeq double [[COND5]], 0.000000e+00
@@ -332,7 +333,8 @@ define void @unfold2(i32 %x, i32 %y) nounwind !prof !1 {
; CHECK: cond.false:
; CHECK-NEXT: [[ADD:%.*]] = add nsw i32 [[X]], [[Y]]
; CHECK-NEXT: [[CMP1:%.*]] = icmp sgt i32 [[ADD]], 10
-; CHECK-NEXT: br i1 [[CMP1]], label [[IF_THEN:%.*]], label [[COND_END4:%.*]], !prof [[PROF1]]
+; CHECK-NEXT: [[COND_FR:%.*]] = freeze i1 [[CMP1]]
+; CHECK-NEXT: br i1 [[COND_FR]], label [[IF_THEN:%.*]], label [[COND_END4:%.*]], !prof [[PROF1]]
; CHECK: cond.end4:
; CHECK-NEXT: [[COND5:%.*]] = phi i32 [ [[ADD]], [[COND_FALSE]] ]
; CHECK-NEXT: [[CMP6:%.*]] = icmp eq i32 [[COND5]], 0
@@ -556,7 +558,8 @@ define void @test_func(ptr nocapture readonly %a, ptr nocapture readonly %b, ptr
; CHECK-NEXT: [[ARRAYIDX5:%.*]] = getelementptr inbounds i32, ptr [[C:%.*]], i64 [[TMP0]]
; CHECK-NEXT: [[TMP3:%.*]] = load i32, ptr [[ARRAYIDX5]], align 4
; CHECK-NEXT: [[CMP6:%.*]] = icmp eq i32 [[TMP2]], [[TMP3]]
-; CHECK-NEXT: br i1 [[CMP6]], label [[SW_BB:%.*]], label [[SW_BB7:%.*]]
+; CHECK-NEXT: [[COND_FR:%.*]] = freeze i1 [[CMP6]]
+; CHECK-NEXT: br i1 [[COND_FR]], label [[SW_BB:%.*]], label [[SW_BB7:%.*]]
; CHECK: if.end:
; CHECK-NEXT: [[LOCAL_VAR_0:%.*]] = phi i32 [ [[TMP1]], [[FOR_BODY]] ]
; CHECK-NEXT: switch i32 [[LOCAL_VAR_0]], label [[SW_DEFAULT]] [
diff --git a/llvm/test/Transforms/JumpThreading/stale-loop-info-after-unfold-select.ll b/llvm/test/Transforms/JumpThreading/stale-loop-info-after-unfold-select.ll
index ba40980c6bf89..09b0807585777 100644
--- a/llvm/test/Transforms/JumpThreading/stale-loop-info-after-unfold-select.ll
+++ b/llvm/test/Transforms/JumpThreading/stale-loop-info-after-unfold-select.ll
@@ -1,29 +1,31 @@
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
; RUN: opt -S -passes='require<loops>,jump-threading,verify<loops>' < %s | FileCheck %s
%"type1" = type { i8 }
%"type2" = type opaque
define dso_local ptr @func2(ptr %this, ptr) {
-; CHECK-LABEL: @func2(
-; CHECK-NEXT: entry:
-; CHECK-NEXT: br label [[WHILE_COND:%.*]]
-; CHECK: select.unfold:
-; CHECK-NEXT: br label [[WHILE_COND]]
-; CHECK: while.cond:
-; CHECK-NEXT: [[MONTH_0:%.*]] = phi i32 [ undef, [[ENTRY:%.*]] ], [ [[CALL2:%.*]], [[FUNC1_EXIT:%.*]] ], [ [[ADD:%.*]], [[SELECT_UNFOLD:%.*]] ]
-; CHECK-NEXT: switch i32 [[MONTH_0]], label [[IF_END_I:%.*]] [
-; CHECK-NEXT: i32 4, label [[FUNC1_EXIT]]
-; CHECK-NEXT: i32 1, label [[FUNC1_EXIT]]
+; CHECK-LABEL: define dso_local ptr @func2(
+; CHECK-SAME: ptr [[THIS:%.*]], ptr [[TMP0:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: br label %[[WHILE_COND:.*]]
+; CHECK: [[SELECT_UNFOLD:.*]]:
+; CHECK-NEXT: br label %[[WHILE_COND]]
+; CHECK: [[WHILE_COND]]:
+; CHECK-NEXT: [[MONTH_0:%.*]] = phi i32 [ undef, %[[ENTRY]] ], [ [[CALL2:%.*]], %[[FUNC1_EXIT:.*]] ], [ [[ADD:%.*]], %[[SELECT_UNFOLD]] ]
+; CHECK-NEXT: switch i32 [[MONTH_0]], label %[[IF_END_I:.*]] [
+; CHECK-NEXT: i32 4, label %[[FUNC1_EXIT]]
+; CHECK-NEXT: i32 1, label %[[FUNC1_EXIT]]
; CHECK-NEXT: ]
-; CHECK: if.end.i:
-; CHECK-NEXT: br label [[FUNC1_EXIT]]
-; CHECK: func1.exit:
-; CHECK-NEXT: [[RETVAL_0_I:%.*]] = phi i32 [ 9, [[IF_END_I]] ], [ 0, [[WHILE_COND]] ], [ 0, [[WHILE_COND]] ]
+; CHECK: [[IF_END_I]]:
+; CHECK-NEXT: br label %[[FUNC1_EXIT]]
+; CHECK: [[FUNC1_EXIT]]:
+; CHECK-NEXT: [[RETVAL_0_I:%.*]] = phi i32 [ 9, %[[IF_END_I]] ], [ 0, %[[WHILE_COND]] ], [ 0, %[[WHILE_COND]] ]
; CHECK-NEXT: [[CALL2]] = tail call signext i32 @func3(i32 signext [[RETVAL_0_I]], i32 signext 1, i32 signext 3)
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 [[CALL2]], 1
; CHECK-NEXT: [[ADD]] = add nsw i32 [[CALL2]], 2
-; CHECK-NEXT: br i1 [[CMP]], label [[SELECT_UNFOLD]], label [[WHILE_COND]]
+; CHECK-NEXT: [[COND_FR:%.*]] = freeze i1 [[CMP]]
+; CHECK-NEXT: br i1 [[COND_FR]], label %[[SELECT_UNFOLD]], label %[[WHILE_COND]]
;
entry:
br label %while.cond
@@ -31,8 +33,8 @@ entry:
while.cond: ; preds = %func1.exit, %entry
%month.0 = phi i32 [ undef, %entry ], [ %month.0.be, %func1.exit ]
switch i32 %month.0, label %if.end.i [
- i32 4, label %func1.exit
- i32 1, label %func1.exit
+ i32 4, label %func1.exit
+ i32 1, label %func1.exit
]
if.end.i: ; preds = %while.cond
@@ -49,3 +51,82 @@ func1.exit: ; preds = %if.end.i, %while.cond, %while.cond
declare i32 @func3(i32, i32, i32)
+declare void @sink(i32)
+
+; Check that unfoldSelectInstr freezes a possibly-undef-or-poison condition
+; before using it as a branch input. A select with an undef or poison condition
+; does not immediately cause UB, but branching on undef or poison does.
+define void @unfold_select_poison_cond(i1 %c, i1 %maybe_poison) {
+; CHECK-LABEL: define void @unfold_select_poison_cond(
+; CHECK-SAME: i1 [[C:%.*]], i1 [[MAYBE_POISON:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: br i1 [[C]], label %[[PRED1:.*]], label %[[IF_ELSE:.*]]
+; CHECK: [[PRED1]]:
+; CHECK-NEXT: [[COND_FR:%.*]] = freeze i1 [[MAYBE_POISON]]
+; CHECK-NEXT: br i1 [[COND_FR]], label %[[IF_THEN:.*]], label %[[IF_ELSE]]
+; CHECK: [[IF_THEN]]:
+; CHECK-NEXT: call void @sink(i32 1)
+; CHECK-NEXT: ret void
+; CHECK: [[IF_ELSE]]:
+; CHECK-NEXT: call void @sink(i32 2)
+; CHECK-NEXT: ret void
+;
+entry:
+ br i1 %c, label %pred1, label %pred2
+
+pred1:
+ %s = select i1 %maybe_poison, i32 0, i32 10
+ br label %merge
+
+pred2:
+ br label %merge
+
+merge:
+ %p = phi i32 [ %s, %pred1 ], [ 5, %pred2 ]
+ %cmp = icmp eq i32 %p, 0
+ %fcmp = freeze i1 %cmp
+ br i1 %fcmp, label %if_then, label %if_else
+
+if_then:
+ call void @sink(i32 1)
+ ret void
+
+if_else:
+ call void @sink(i32 2)
+ ret void
+}
+
+define void @unfold_select_poison_cond_2(i1 %c) {
+; CHECK-LABEL: define void @unfold_select_poison_cond_2(
+; CHECK-SAME: i1 [[C:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: br label %[[LOOP:.*]]
+; CHECK: [[LOOP]]:
+; CHECK-NEXT: [[IV:%.*]] = phi i64 [ 0, %[[ENTRY]] ], [ 0, %[[LATCH:.*]] ]
+; CHECK-NEXT: br i1 [[C]], label %[[LATCH]], label %[[LATCH]]
+; CHECK: [[LATCH]]:
+; CHECK-NEXT: br label %[[LOOP]]
+;
+entry:
+ br label %loop
+
+loop:
+ %iv = phi i64 [ 0, %entry ], [ %iv.next, %latch ]
+ br i1 %c, label %merge, label %if.then
+
+if.then:
+ %cmp = icmp eq i64 %iv, 0
+ %sel = select i1 %cmp, i32 0, i32 0
+ br label %merge
+
+merge:
+ %iv.next = phi i64 [ 0, %if.then ], [ 0, %loop ]
+ %val = phi i32 [ %sel, %if.then ], [ 0, %loop ]
+ switch i32 %val, label %latch [
+ i32 0, label %latch
+ i32 6, label %latch
+ ]
+
+latch:
+ br label %loop
+}
|
You can test this locally with the following command:git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef([^a-zA-Z0-9_-]|$)|UndefValue::get)' 'HEAD~1' HEAD llvm/lib/Transforms/Scalar/JumpThreading.cpp llvm/test/Transforms/JumpThreading/branch-debug-info2.ll llvm/test/Transforms/JumpThreading/select.ll llvm/test/Transforms/JumpThreading/stale-loop-info-after-unfold-select.llThe following files introduce new uses of undef:
Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields In tests, avoid using For example, this is considered a bad practice: define void @fn() {
...
br i1 undef, ...
}Please use the following instead: define void @fn(i1 %cond) {
...
br i1 %cond, ...
}Please refer to the Undefined Behavior Manual for more information. |
Contributor
boomanaiden154
left a comment
boomanaiden154
left a comment
There was a problem hiding this comment.
What does this PR change to address the performance regressions from the previous attempt?
Member
|
Good catch. It seems that similar problem exists in DFAJumpThreading too: llvm-project/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp Lines 267 to 269 in 1b19ecc |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When turning a select into a branch, make sure we freeze the condition when it may be poison or undef.
Original patch: #199408.
Fixes: #199702.
Co-authored-by: Justin Lebar justin.lebar@gmail.com