[llvm] 53500e3 - Reapply [SimplifyCFG][LICM] Preserve nonnull, range and align metadata when speculating
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 20 05:17:29 PDT 2023
Author: Nikita Popov
Date: 2023-04-20T14:17:15+02:00
New Revision: 53500e333d35062942065d55364c45d97f03eac0
URL: https://github.com/llvm/llvm-project/commit/53500e333d35062942065d55364c45d97f03eac0
DIFF: https://github.com/llvm/llvm-project/commit/53500e333d35062942065d55364c45d97f03eac0.diff
LOG: Reapply [SimplifyCFG][LICM] Preserve nonnull, range and align metadata when speculating
This exposed another miscompile in GVN, which was fixed by
20e9b31f88149a1d5ef78c0be50051e345098e41.
-----
After D141386, violation of nonnull, range and align metadata
results in poison rather than immediate undefined behavior,
which means that these are now safe to retain when speculating.
We only need to remove UB-implying metadata like noundef.
This is done by adding a dropUBImplyingAttrsAndMetadata() helper,
which lists the metadata which is known safe to retain on speculation.
Differential Revision: https://reviews.llvm.org/D146629
Added:
Modified:
llvm/include/llvm/IR/Instruction.h
llvm/lib/IR/Instruction.cpp
llvm/lib/Transforms/Scalar/LICM.cpp
llvm/lib/Transforms/Scalar/SROA.cpp
llvm/lib/Transforms/Utils/Local.cpp
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
llvm/test/Transforms/LICM/hoist-metadata.ll
llvm/test/Transforms/SimplifyCFG/hoist-with-metadata.ll
Removed:
################################################################################
diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index 1d82f5d4b3122..3a3278260cf59 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -401,11 +401,16 @@ class Instruction : public User,
}
/// This function drops non-debug unknown metadata (through
- /// dropUnknownNonDebugMetadata). For calls, it also drops parameter and
+ /// dropUnknownNonDebugMetadata). For calls, it also drops parameter and
/// return attributes that can cause undefined behaviour. Both of these should
/// be done by passes which move instructions in IR.
void dropUBImplyingAttrsAndUnknownMetadata(ArrayRef<unsigned> KnownIDs = {});
+ /// Drop any attributes or metadata that can cause immediate undefined
+ /// behavior. Retain other attributes/metadata on a best-effort basis.
+ /// This should be used when speculating instructions.
+ void dropUBImplyingAttrsAndMetadata();
+
/// Determine whether the exact flag is set.
bool isExact() const LLVM_READONLY;
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 84db9558d259d..42efaede16351 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -243,6 +243,16 @@ void Instruction::dropUBImplyingAttrsAndUnknownMetadata(
CB->removeRetAttrs(UBImplyingAttributes);
}
+void Instruction::dropUBImplyingAttrsAndMetadata() {
+ // !annotation metadata does not impact semantics.
+ // !range, !nonnull and !align produce poison, so they are safe to speculate.
+ // !noundef and various AA metadata must be dropped, as it generally produces
+ // immediate undefined behavior.
+ unsigned KnownIDs[] = {LLVMContext::MD_annotation, LLVMContext::MD_range,
+ LLVMContext::MD_nonnull, LLVMContext::MD_align};
+ dropUBImplyingAttrsAndUnknownMetadata(KnownIDs);
+}
+
bool Instruction::isExact() const {
return cast<PossiblyExactOperator>(this)->isExact();
}
diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index 6742151bb4e47..b5298cfd258bd 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -1757,7 +1757,7 @@ static void hoist(Instruction &I, const DominatorTree *DT, const Loop *CurLoop,
// time in isGuaranteedToExecute if we don't actually have anything to
// drop. It is a compile time optimization, not required for correctness.
!SafetyInfo->isGuaranteedToExecute(I, DT, CurLoop))
- I.dropUBImplyingAttrsAndUnknownMetadata();
+ I.dropUBImplyingAttrsAndMetadata();
if (isa<PHINode>(I))
// Move the new node to the end of the phi list in the destination block.
diff --git a/llvm/lib/Transforms/Scalar/SROA.cpp b/llvm/lib/Transforms/Scalar/SROA.cpp
index db518a4c101eb..cecd4acc129c6 100644
--- a/llvm/lib/Transforms/Scalar/SROA.cpp
+++ b/llvm/lib/Transforms/Scalar/SROA.cpp
@@ -1697,7 +1697,7 @@ static void rewriteMemOpOfSelect(SelectInst &SI, T &I,
else
++NumStoresPredicated;
} else {
- CondMemOp.dropUBImplyingAttrsAndUnknownMetadata();
+ CondMemOp.dropUBImplyingAttrsAndMetadata();
++NumLoadsSpeculated;
}
CondMemOp.insertBefore(NewMemOpBB->getTerminator());
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index 688cb20d7380c..d8036f7299a59 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -2990,7 +2990,7 @@ void llvm::hoistAllInstructionsInto(BasicBlock *DomBlock, Instruction *InsertPt,
for (BasicBlock::iterator II = BB->begin(), IE = BB->end(); II != IE;) {
Instruction *I = &*II;
- I->dropUBImplyingAttrsAndUnknownMetadata();
+ I->dropUBImplyingAttrsAndMetadata();
if (I->isUsedByMetadata())
dropDebugUsers(*I);
if (I->isDebugOrPseudoInst()) {
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index ed39144a1e10e..137fdf192831b 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -1117,16 +1117,12 @@ static void CloneInstructionsIntoPredecessorBlockAndUpdateSSAUses(
RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
VMap[&BonusInst] = NewBonusInst;
- // If we moved a load, we cannot any longer claim any knowledge about
- // its potential value. The previous information might have been valid
+ // If we speculated an instruction, we need to drop any metadata that may
+ // result in undefined behavior, as the metadata might have been valid
// only given the branch precondition.
- // For an analogous reason, we must also drop all the metadata whose
- // semantics we don't understand. We *can* preserve !annotation, because
- // it is tied to the instruction itself, not the value or position.
// Similarly strip attributes on call parameters that may cause UB in
// location the call is moved to.
- NewBonusInst->dropUBImplyingAttrsAndUnknownMetadata(
- LLVMContext::MD_annotation);
+ NewBonusInst->dropUBImplyingAttrsAndMetadata();
NewBonusInst->insertInto(PredBlock, PTI->getIterator());
NewBonusInst->takeName(&BonusInst);
@@ -3014,7 +3010,7 @@ bool SimplifyCFGOpt::SpeculativelyExecuteBB(BranchInst *BI, BasicBlock *ThenBB,
}
// Metadata can be dependent on the condition we are hoisting above.
- // Conservatively strip all metadata on the instruction. Drop the debug loc
+ // Strip all UB-implying metadata on the instruction. Drop the debug loc
// to avoid making it appear as if the condition is a constant, which would
// be misleading while debugging.
// Similarly strip attributes that maybe dependent on condition we are
@@ -3025,7 +3021,7 @@ bool SimplifyCFGOpt::SpeculativelyExecuteBB(BranchInst *BI, BasicBlock *ThenBB,
if (!isa<DbgAssignIntrinsic>(&I))
I.setDebugLoc(DebugLoc());
}
- I.dropUBImplyingAttrsAndUnknownMetadata();
+ I.dropUBImplyingAttrsAndMetadata();
// Drop ephemeral values.
if (EphTracker.contains(&I)) {
diff --git a/llvm/test/Transforms/LICM/hoist-metadata.ll b/llvm/test/Transforms/LICM/hoist-metadata.ll
index 7c5ed1f7ae6fb..af4cc314a0599 100644
--- a/llvm/test/Transforms/LICM/hoist-metadata.ll
+++ b/llvm/test/Transforms/LICM/hoist-metadata.ll
@@ -3,6 +3,7 @@
declare void @foo(...) memory(none)
+; We can preserve all metadata on instructions that are guaranteed to execute.
define void @test_unconditional(i1 %c, ptr dereferenceable(8) align 8 %p) {
; CHECK-LABEL: define void @test_unconditional
; CHECK-SAME: (i1 [[C:%.*]], ptr align 8 dereferenceable(8) [[P:%.*]]) {
@@ -29,12 +30,14 @@ exit:
ret void
}
+; We cannot preserve UB-implying metadata on instructions that are speculated.
+; However, we can preserve poison-implying metadata.
define void @test_conditional(i1 %c, i1 %c2, ptr dereferenceable(8) align 8 %p) {
; CHECK-LABEL: define void @test_conditional
; CHECK-SAME: (i1 [[C:%.*]], i1 [[C2:%.*]], ptr align 8 dereferenceable(8) [[P:%.*]]) {
-; CHECK-NEXT: [[V1:%.*]] = load i32, ptr [[P]], align 4
-; CHECK-NEXT: [[V2:%.*]] = load ptr, ptr [[P]], align 8
-; CHECK-NEXT: [[V3:%.*]] = load ptr, ptr [[P]], align 8
+; CHECK-NEXT: [[V1:%.*]] = load i32, ptr [[P]], align 4, !range [[RNG0]]
+; CHECK-NEXT: [[V2:%.*]] = load ptr, ptr [[P]], align 8, !nonnull !1
+; CHECK-NEXT: [[V3:%.*]] = load ptr, ptr [[P]], align 8, !align !2
; CHECK-NEXT: br label [[LOOP:%.*]]
; CHECK: loop:
; CHECK-NEXT: br i1 [[C]], label [[IF:%.*]], label [[LATCH:%.*]]
diff --git a/llvm/test/Transforms/SimplifyCFG/hoist-with-metadata.ll b/llvm/test/Transforms/SimplifyCFG/hoist-with-metadata.ll
index a495b3ee89e85..b999b07fc24b4 100644
--- a/llvm/test/Transforms/SimplifyCFG/hoist-with-metadata.ll
+++ b/llvm/test/Transforms/SimplifyCFG/hoist-with-metadata.ll
@@ -97,10 +97,11 @@ out:
ret void
}
+; !range violation only returns poison, and is thus safe to speculate.
define i32 @speculate_range(i1 %c, ptr dereferenceable(8) align 8 %p) {
; CHECK-LABEL: @speculate_range(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[V:%.*]] = load i32, ptr [[P:%.*]], align 4
+; CHECK-NEXT: [[V:%.*]] = load i32, ptr [[P:%.*]], align 4, !range [[RNG3:![0-9]+]]
; CHECK-NEXT: [[SPEC_SELECT:%.*]] = select i1 [[C:%.*]], i32 [[V]], i32 0
; CHECK-NEXT: ret i32 [[SPEC_SELECT]]
;
@@ -116,10 +117,12 @@ join:
ret i32 %phi
}
+; !nonnull is safe to speculate, but !noundef is not, as the latter causes
+; immediate undefined behavior.
define ptr @speculate_nonnull(i1 %c, ptr dereferenceable(8) align 8 %p) {
; CHECK-LABEL: @speculate_nonnull(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[V:%.*]] = load ptr, ptr [[P:%.*]], align 8
+; CHECK-NEXT: [[V:%.*]] = load ptr, ptr [[P:%.*]], align 8, !nonnull !1
; CHECK-NEXT: [[SPEC_SELECT:%.*]] = select i1 [[C:%.*]], ptr [[V]], ptr null
; CHECK-NEXT: ret ptr [[SPEC_SELECT]]
;
@@ -135,11 +138,12 @@ join:
ret ptr %phi
}
-
+; !align is safe to speculate, but !dereferenceable is not, as the latter causes
+; immediate undefined behavior.
define ptr @speculate_align(i1 %c, ptr dereferenceable(8) align 8 %p) {
; CHECK-LABEL: @speculate_align(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[V:%.*]] = load ptr, ptr [[P:%.*]], align 8
+; CHECK-NEXT: [[V:%.*]] = load ptr, ptr [[P:%.*]], align 8, !align !4
; CHECK-NEXT: [[SPEC_SELECT:%.*]] = select i1 [[C:%.*]], ptr [[V]], ptr null
; CHECK-NEXT: ret ptr [[SPEC_SELECT]]
;
@@ -158,7 +162,7 @@ join:
define void @hoist_fpmath(i1 %c, double %x) {
; CHECK-LABEL: @hoist_fpmath(
; CHECK-NEXT: if:
-; CHECK-NEXT: [[T:%.*]] = fadd double [[X:%.*]], 1.000000e+00, !fpmath !3
+; CHECK-NEXT: [[T:%.*]] = fadd double [[X:%.*]], 1.000000e+00, !fpmath !5
; CHECK-NEXT: ret void
;
if:
@@ -180,5 +184,7 @@ out:
; CHECK: [[RNG0]] = !{i8 0, i8 1, i8 3, i8 5}
; CHECK: [[META1:![0-9]+]] = !{}
; CHECK: [[META2:![0-9]+]] = !{i64 10}
-; CHECK: [[META3:![0-9]+]] = !{float 2.500000e+00}
+; CHECK: [[RNG3]] = !{i32 0, i32 10}
+; CHECK: [[META4:![0-9]+]] = !{i64 4}
+; CHECK: [[META5:![0-9]+]] = !{float 2.500000e+00}
;.
More information about the llvm-commits
mailing list