[llvm] bf7f6b4 - Revert "Reapply [SimplifyCFG][LICM] Preserve nonnull, range and align metadata when speculating"
Krasimir Georgiev via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 19 01:29:23 PDT 2023
Author: Krasimir Georgiev
Date: 2023-04-19T08:28:48Z
New Revision: bf7f6b44362c9dae055328684c2ca38cd0937972
URL: https://github.com/llvm/llvm-project/commit/bf7f6b44362c9dae055328684c2ca38cd0937972
DIFF: https://github.com/llvm/llvm-project/commit/bf7f6b44362c9dae055328684c2ca38cd0937972.diff
LOG: Revert "Reapply [SimplifyCFG][LICM] Preserve nonnull, range and align metadata when speculating"
This reverts commit 6f7e5c0f1ac6cc3349a2e1479ac4208465b272c6.
Seems to expose a miscompile in rust, possibly exposing a bug in LLVM
somewhere. Investigation thread over at:
https://rust-lang.zulipchat.com/#narrow/stream/187780-t-compiler.2Fwg-llvm/topic/LLVM.20D146629.20breakage
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 3a3278260cf59..1d82f5d4b3122 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -401,16 +401,11 @@ 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 42efaede16351..84db9558d259d 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -243,16 +243,6 @@ 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 b5298cfd258bd..6742151bb4e47 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.dropUBImplyingAttrsAndMetadata();
+ I.dropUBImplyingAttrsAndUnknownMetadata();
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 db60e3b03d637..d5615c902ad15 100644
--- a/llvm/lib/Transforms/Scalar/SROA.cpp
+++ b/llvm/lib/Transforms/Scalar/SROA.cpp
@@ -1696,7 +1696,7 @@ static void rewriteMemOpOfSelect(SelectInst &SI, T &I,
else
++NumStoresPredicated;
} else {
- CondMemOp.dropUBImplyingAttrsAndMetadata();
+ CondMemOp.dropUBImplyingAttrsAndUnknownMetadata();
++NumLoadsSpeculated;
}
CondMemOp.insertBefore(NewMemOpBB->getTerminator());
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index d8036f7299a59..688cb20d7380c 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->dropUBImplyingAttrsAndMetadata();
+ I->dropUBImplyingAttrsAndUnknownMetadata();
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 137fdf192831b..ed39144a1e10e 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -1117,12 +1117,16 @@ static void CloneInstructionsIntoPredecessorBlockAndUpdateSSAUses(
RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
VMap[&BonusInst] = NewBonusInst;
- // If we speculated an instruction, we need to drop any metadata that may
- // result in undefined behavior, as the metadata might have been valid
+ // If we moved a load, we cannot any longer claim any knowledge about
+ // its potential value. The previous information 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->dropUBImplyingAttrsAndMetadata();
+ NewBonusInst->dropUBImplyingAttrsAndUnknownMetadata(
+ LLVMContext::MD_annotation);
NewBonusInst->insertInto(PredBlock, PTI->getIterator());
NewBonusInst->takeName(&BonusInst);
@@ -3010,7 +3014,7 @@ bool SimplifyCFGOpt::SpeculativelyExecuteBB(BranchInst *BI, BasicBlock *ThenBB,
}
// Metadata can be dependent on the condition we are hoisting above.
- // Strip all UB-implying metadata on the instruction. Drop the debug loc
+ // Conservatively strip all 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
@@ -3021,7 +3025,7 @@ bool SimplifyCFGOpt::SpeculativelyExecuteBB(BranchInst *BI, BasicBlock *ThenBB,
if (!isa<DbgAssignIntrinsic>(&I))
I.setDebugLoc(DebugLoc());
}
- I.dropUBImplyingAttrsAndMetadata();
+ I.dropUBImplyingAttrsAndUnknownMetadata();
// 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 af4cc314a0599..7c5ed1f7ae6fb 100644
--- a/llvm/test/Transforms/LICM/hoist-metadata.ll
+++ b/llvm/test/Transforms/LICM/hoist-metadata.ll
@@ -3,7 +3,6 @@
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:%.*]]) {
@@ -30,14 +29,12 @@ 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, !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: [[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: 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 b999b07fc24b4..a495b3ee89e85 100644
--- a/llvm/test/Transforms/SimplifyCFG/hoist-with-metadata.ll
+++ b/llvm/test/Transforms/SimplifyCFG/hoist-with-metadata.ll
@@ -97,11 +97,10 @@ 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, !range [[RNG3:![0-9]+]]
+; CHECK-NEXT: [[V:%.*]] = load i32, ptr [[P:%.*]], align 4
; CHECK-NEXT: [[SPEC_SELECT:%.*]] = select i1 [[C:%.*]], i32 [[V]], i32 0
; CHECK-NEXT: ret i32 [[SPEC_SELECT]]
;
@@ -117,12 +116,10 @@ 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, !nonnull !1
+; CHECK-NEXT: [[V:%.*]] = load ptr, ptr [[P:%.*]], align 8
; CHECK-NEXT: [[SPEC_SELECT:%.*]] = select i1 [[C:%.*]], ptr [[V]], ptr null
; CHECK-NEXT: ret ptr [[SPEC_SELECT]]
;
@@ -138,12 +135,11 @@ 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, !align !4
+; CHECK-NEXT: [[V:%.*]] = load ptr, ptr [[P:%.*]], align 8
; CHECK-NEXT: [[SPEC_SELECT:%.*]] = select i1 [[C:%.*]], ptr [[V]], ptr null
; CHECK-NEXT: ret ptr [[SPEC_SELECT]]
;
@@ -162,7 +158,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 !5
+; CHECK-NEXT: [[T:%.*]] = fadd double [[X:%.*]], 1.000000e+00, !fpmath !3
; CHECK-NEXT: ret void
;
if:
@@ -184,7 +180,5 @@ out:
; CHECK: [[RNG0]] = !{i8 0, i8 1, i8 3, i8 5}
; CHECK: [[META1:![0-9]+]] = !{}
; CHECK: [[META2:![0-9]+]] = !{i64 10}
-; CHECK: [[RNG3]] = !{i32 0, i32 10}
-; CHECK: [[META4:![0-9]+]] = !{i64 4}
-; CHECK: [[META5:![0-9]+]] = !{float 2.500000e+00}
+; CHECK: [[META3:![0-9]+]] = !{float 2.500000e+00}
;.
More information about the llvm-commits
mailing list