[llvm] 2895c4c - [ObjC][ARC] Fix non-deterministic behavior in ProvenanceAnalysis
Akira Hatanaka via llvm-commits
llvm-commits at lists.llvm.org
Thu May 11 13:41:28 PDT 2023
Author: Akira Hatanaka
Date: 2023-05-11T13:40:43-07:00
New Revision: 2895c4c00ff66420a2232f3fcfc341eb7b72ce67
URL: https://github.com/llvm/llvm-project/commit/2895c4c00ff66420a2232f3fcfc341eb7b72ce67
DIFF: https://github.com/llvm/llvm-project/commit/2895c4c00ff66420a2232f3fcfc341eb7b72ce67.diff
LOG: [ObjC][ARC] Fix non-deterministic behavior in ProvenanceAnalysis
Stop reordering the pointers passed in ProvenanceAnalysis::related based
on their values. That was causing non-determinism as the call to
relatedCheck(A, B) isn't guaranteed to return the same result as
relatedCheck(B, A).
Revert the following three commits (except the original test case in
related-check.ll):
665e47777df17db406c698d57b4f3c28d67c432e
295861514e0d1e48df2918b630dd692ac27ee0de
d877e3fe71676b0ff10410d80456b35cdd5bf796
These changes shouldn't be necessary once the call to std::swap is
removed.
Differential Revision: https://reviews.llvm.org/D150296
Added:
Modified:
llvm/lib/Transforms/ObjCARC/ProvenanceAnalysis.cpp
llvm/test/Transforms/ObjCARC/related-check.ll
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/ObjCARC/ProvenanceAnalysis.cpp b/llvm/lib/Transforms/ObjCARC/ProvenanceAnalysis.cpp
index 2fa25a79ae9d5..23855231c5b98 100644
--- a/llvm/lib/Transforms/ObjCARC/ProvenanceAnalysis.cpp
+++ b/llvm/lib/Transforms/ObjCARC/ProvenanceAnalysis.cpp
@@ -42,40 +42,21 @@ bool ProvenanceAnalysis::relatedSelect(const SelectInst *A,
const Value *B) {
// If the values are Selects with the same condition, we can do a more precise
// check: just check for relations between the values on corresponding arms.
- if (const SelectInst *SB = dyn_cast<SelectInst>(B)) {
+ if (const SelectInst *SB = dyn_cast<SelectInst>(B))
if (A->getCondition() == SB->getCondition())
return related(A->getTrueValue(), SB->getTrueValue()) ||
related(A->getFalseValue(), SB->getFalseValue());
- // Check both arms of B individually. Return false if neither arm is related
- // to A.
- if (!(related(SB->getTrueValue(), A) || related(SB->getFalseValue(), A)))
- return false;
- }
-
// Check both arms of the Select node individually.
return related(A->getTrueValue(), B) || related(A->getFalseValue(), B);
}
bool ProvenanceAnalysis::relatedPHI(const PHINode *A,
const Value *B) {
-
- auto comparePHISources = [this](const PHINode *PNA, const Value *B) -> bool {
- // Check each unique source of the PHI node against B.
- SmallPtrSet<const Value *, 4> UniqueSrc;
- for (Value *PV1 : PNA->incoming_values()) {
- if (UniqueSrc.insert(PV1).second && related(PV1, B))
- return true;
- }
-
- // All of the arms checked out.
- return false;
- };
-
- if (const PHINode *PNB = dyn_cast<PHINode>(B)) {
- // If the values are PHIs in the same block, we can do a more precise as
- // well as efficient check: just check for relations between the values on
- // corresponding edges.
+ // If the values are PHIs in the same block, we can do a more precise as well
+ // as efficient check: just check for relations between the values on
+ // corresponding edges.
+ if (const PHINode *PNB = dyn_cast<PHINode>(B))
if (PNB->getParent() == A->getParent()) {
for (unsigned i = 0, e = A->getNumIncomingValues(); i != e; ++i)
if (related(A->getIncomingValue(i),
@@ -84,11 +65,15 @@ bool ProvenanceAnalysis::relatedPHI(const PHINode *A,
return false;
}
- if (!comparePHISources(PNB, A))
- return false;
+ // Check each unique source of the PHI node against B.
+ SmallPtrSet<const Value *, 4> UniqueSrc;
+ for (Value *PV1 : A->incoming_values()) {
+ if (UniqueSrc.insert(PV1).second && related(PV1, B))
+ return true;
}
- return comparePHISources(A, B);
+ // All of the arms checked out.
+ return false;
}
/// Test if the value of P, or any value covered by its provenance, is ever
@@ -140,19 +125,22 @@ bool ProvenanceAnalysis::relatedCheck(const Value *A, const Value *B) {
bool BIsIdentified = IsObjCIdentifiedObject(B);
// An ObjC-Identified object can't alias a load if it is never locally stored.
-
- // Check for an obvious escape.
- if ((AIsIdentified && isa<LoadInst>(B) && !IsStoredObjCPointer(A)) ||
- (BIsIdentified && isa<LoadInst>(A) && !IsStoredObjCPointer(B)))
- return false;
-
- if ((AIsIdentified && isa<LoadInst>(B)) ||
- (BIsIdentified && isa<LoadInst>(A)))
- return true;
-
- // Both pointers are identified and escapes aren't an evident problem.
- if (AIsIdentified && BIsIdentified && !isa<LoadInst>(A) && !isa<LoadInst>(B))
- return false;
+ if (AIsIdentified) {
+ // Check for an obvious escape.
+ if (isa<LoadInst>(B))
+ return IsStoredObjCPointer(A);
+ if (BIsIdentified) {
+ // Check for an obvious escape.
+ if (isa<LoadInst>(A))
+ return IsStoredObjCPointer(B);
+ // Both pointers are identified and escapes aren't an evident problem.
+ return false;
+ }
+ } else if (BIsIdentified) {
+ // Check for an obvious escape.
+ if (isa<LoadInst>(A))
+ return IsStoredObjCPointer(B);
+ }
// Special handling for PHI and Select.
if (const PHINode *PN = dyn_cast<PHINode>(A))
@@ -179,15 +167,12 @@ bool ProvenanceAnalysis::related(const Value *A, const Value *B) {
// Begin by inserting a conservative value into the map. If the insertion
// fails, we have the answer already. If it succeeds, leave it there until we
// compute the real answer to guard against recursive queries.
- if (A > B) std::swap(A, B);
std::pair<CachedResultsTy::iterator, bool> Pair =
CachedResults.insert(std::make_pair(ValuePairTy(A, B), true));
if (!Pair.second)
return Pair.first->second;
bool Result = relatedCheck(A, B);
- assert(relatedCheck(B, A) == Result &&
- "relatedCheck result depending on order of parameters!");
CachedResults[ValuePairTy(A, B)] = Result;
return Result;
}
diff --git a/llvm/test/Transforms/ObjCARC/related-check.ll b/llvm/test/Transforms/ObjCARC/related-check.ll
index 6eb96a696d23b..7c56b2df5a5a9 100644
--- a/llvm/test/Transforms/ObjCARC/related-check.ll
+++ b/llvm/test/Transforms/ObjCARC/related-check.ll
@@ -117,38 +117,6 @@ if.end19: ; preds = %if.end18, %for.body
br i1 %exitcond.not, label %for.cond.cleanup.loopexit, label %for.body
}
-; CHECK-LABEL: define ptr @foo() {
-; CHECK-NOT: @llvm.objc
-; CHECK: ret ptr %
-
-define ptr @foo() {
- %t = alloca ptr, align 8
- %v4 = load ptr, ptr @global1, align 8
- %v5 = tail call ptr @llvm.objc.retain(ptr %v4)
- store ptr %v4, ptr %t, align 8
- %v13 = load ptr, ptr @"OBJC_CLASSLIST_REFERENCES_$_", align 8
- %call78 = call ptr @bar(ptr %v13)
- call void @llvm.objc.release(ptr %v4)
- ret ptr %call78
-}
-
-; CHECK-LABEL: define void @test_select(
-; CHECK: call ptr @llvm.objc.retain(
-; CHECK: call void @llvm.objc.release(
-
-define void @test_select(i1 %c0, i1 %c1, ptr %p0, ptr %p1) {
- %cond = select i1 %c0, ptr %p0, ptr %p1
- %cond5 = select i1 %c0, ptr %p1, ptr %p0
- %cond14 = select i1 %c1, ptr %cond5, ptr null
- call ptr @llvm.objc.retain(ptr %cond14)
- call void @llvm.objc.release(ptr %cond)
- ret void
-}
-
-declare ptr @bar(ptr)
-
-declare ptr @llvm.objc.retain(ptr)
-
; Function Attrs: argmemonly mustprogress nocallback nofree nosync nounwind willreturn
declare void @llvm.lifetime.start.p0(i64 immarg, ptr nocapture) #2
More information about the llvm-commits
mailing list