From af48351cc8fc63148e8cfd10a936b2d5d0c2f078 Mon Sep 17 00:00:00 2001 From: Johannes Doerfert Date: Tue, 12 May 2020 20:51:21 -0500 Subject: [PATCH] [Attributor][FIX] Stabilize the state of AAReturnedValues each update For AAReturnedValues we treated new and existing information differently in the updateImpl. Only the latter was properly analyzed and categorized. The former was thought to be analyzed in the subsequent update. Since the Attributor does not support "self-updates" we need to make sure the state is "stable" after each updateImpl invocation. That is, if the surrounding information does not change, the state is valid. Now we make sure all return values have been handled and properly categorized each iteration. We might not update again if we have not requested a non-fix attribute so we cannot "wait" for the next update to analyze a new return value. Bug reported by @sdmitriev. --- llvm/lib/Transforms/IPO/AttributorAttributes.cpp | 46 +++++++++++++++-------- llvm/test/Transforms/Attributor/returned.ll | 47 +++++++++++++++++++++++- 2 files changed, 75 insertions(+), 18 deletions(-) diff --git a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp index c764e9f..aa459c4 100644 --- a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp +++ b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp @@ -1009,20 +1009,23 @@ ChangeStatus AAReturnedValuesImpl::updateImpl(Attributor &A) { return indicatePessimisticFixpoint(); // Once returned values "directly" present in the code are handled we try to - // resolve returned calls. + // resolve returned calls. To avoid modifications to the ReturnedValues map + // while we iterate over it we kept record of potential new entries in a copy + // map, NewRVsMap. decltype(ReturnedValues) NewRVsMap; - for (auto &It : ReturnedValues) { - LLVM_DEBUG(dbgs() << "[AAReturnedValues] Returned value: " << *It.first - << " by #" << It.second.size() << " RIs\n"); - CallBase *CB = dyn_cast(It.first); + + auto HandleReturnValue = [&](Value *RV, SmallSetVector &RIs) { + LLVM_DEBUG(dbgs() << "[AAReturnedValues] Returned value: " << *RV + << " by #" << RIs.size() << " RIs\n"); + CallBase *CB = dyn_cast(RV); if (!CB || UnresolvedCalls.count(CB)) - continue; + return; if (!CB->getCalledFunction()) { LLVM_DEBUG(dbgs() << "[AAReturnedValues] Unresolved call: " << *CB << "\n"); UnresolvedCalls.insert(CB); - continue; + return; } // TODO: use the function scope once we have call site AAReturnedValues. @@ -1037,7 +1040,7 @@ ChangeStatus AAReturnedValuesImpl::updateImpl(Attributor &A) { LLVM_DEBUG(dbgs() << "[AAReturnedValues] Unresolved call: " << *CB << "\n"); UnresolvedCalls.insert(CB); - continue; + return; } // Do not try to learn partial information. If the callee has unresolved @@ -1045,7 +1048,7 @@ ChangeStatus AAReturnedValuesImpl::updateImpl(Attributor &A) { auto &RetValAAUnresolvedCalls = RetValAA.getUnresolvedCalls(); if (!RetValAAUnresolvedCalls.empty()) { UnresolvedCalls.insert(CB); - continue; + return; } // Now check if we can track transitively returned values. If possible, thus @@ -1067,14 +1070,14 @@ ChangeStatus AAReturnedValuesImpl::updateImpl(Attributor &A) { } if (Unresolved) - continue; + return; // Now track transitively returned values. unsigned &NumRetAA = NumReturnedValuesPerKnownAA[CB]; if (NumRetAA == RetValAA.getNumReturnValues()) { LLVM_DEBUG(dbgs() << "[AAReturnedValues] Skip call as it has not " "changed since it was seen last\n"); - continue; + return; } NumRetAA = RetValAA.getNumReturnValues(); @@ -1093,21 +1096,32 @@ ChangeStatus AAReturnedValuesImpl::updateImpl(Attributor &A) { continue; } else if (isa(RetVal)) { // Constants are valid everywhere, we can simply take them. - NewRVsMap[RetVal].insert(It.second.begin(), It.second.end()); + NewRVsMap[RetVal].insert(RIs.begin(), RIs.end()); continue; } } - } + }; + + for (auto &It : ReturnedValues) + HandleReturnValue(It.first, It.second); + + // Because processing the new information can again lead to new return values + // we have to be careful and iterate until this iteration is complete. The + // idea is that we are in a stable state at the end of an update. All return + // values have been handled and properly categorized. We might not update + // again if we have not requested a non-fix attribute so we cannot "wait" for + // the next update to analyze a new return value. + while (!NewRVsMap.empty()) { + auto It = std::move(NewRVsMap.back()); + NewRVsMap.pop_back(); - // To avoid modifications to the ReturnedValues map while we iterate over it - // we kept record of potential new entries in a copy map, NewRVsMap. - for (auto &It : NewRVsMap) { assert(!It.second.empty() && "Entry does not add anything."); auto &ReturnInsts = ReturnedValues[It.first]; for (ReturnInst *RI : It.second) if (ReturnInsts.insert(RI)) { LLVM_DEBUG(dbgs() << "[AAReturnedValues] Add new returned value " << *It.first << " => " << *RI << "\n"); + HandleReturnValue(It.first, ReturnInsts); Changed = true; } } diff --git a/llvm/test/Transforms/Attributor/returned.ll b/llvm/test/Transforms/Attributor/returned.ll index cdc67b7..1d14685 100644 --- a/llvm/test/Transforms/Attributor/returned.ll +++ b/llvm/test/Transforms/Attributor/returned.ll @@ -1,6 +1,6 @@ ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes -; RUN: opt -attributor -attributor-manifest-internal -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=15 -S < %s | FileCheck %s --check-prefixes=CHECK,NOT_CGSCC_NPM,NOT_CGSCC_OPM,NOT_TUNIT_NPM,IS__TUNIT____,IS________OPM,IS__TUNIT_OPM -; RUN: opt -aa-pipeline=basic-aa -passes=attributor -attributor-manifest-internal -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=15 -S < %s | FileCheck %s --check-prefixes=CHECK,NOT_CGSCC_OPM,NOT_CGSCC_NPM,NOT_TUNIT_OPM,IS__TUNIT____,IS________NPM,IS__TUNIT_NPM +; RUN: opt -attributor -attributor-manifest-internal -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=14 -S < %s | FileCheck %s --check-prefixes=CHECK,NOT_CGSCC_NPM,NOT_CGSCC_OPM,NOT_TUNIT_NPM,IS__TUNIT____,IS________OPM,IS__TUNIT_OPM +; RUN: opt -aa-pipeline=basic-aa -passes=attributor -attributor-manifest-internal -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=14 -S < %s | FileCheck %s --check-prefixes=CHECK,NOT_CGSCC_OPM,NOT_CGSCC_NPM,NOT_TUNIT_OPM,IS__TUNIT____,IS________NPM,IS__TUNIT_NPM ; RUN: opt -attributor-cgscc -attributor-manifest-internal -attributor-annotate-decl-cs -S < %s | FileCheck %s --check-prefixes=CHECK,NOT_TUNIT_NPM,NOT_TUNIT_OPM,NOT_CGSCC_NPM,IS__CGSCC____,IS________OPM,IS__CGSCC_OPM ; RUN: opt -aa-pipeline=basic-aa -passes=attributor-cgscc -attributor-manifest-internal -attributor-annotate-decl-cs -S < %s | FileCheck %s --check-prefixes=CHECK,NOT_TUNIT_NPM,NOT_TUNIT_OPM,NOT_CGSCC_OPM,IS__CGSCC____,IS________NPM,IS__CGSCC_NPM ; @@ -1234,4 +1234,47 @@ define i32* @dont_use_const() #0 { ret i32* %c } +; UTC_ARGS: --disable +; +; Verify we do not derive constraints for @_Z3fooP1X as if it was returning `null`. +; +; CHEKC-NOT: noalias +; CHECK-NOT: align 536870912 + +%struct.Y = type { %struct.X } +%struct.X = type { i32 (...)** } + +@_ZTI1X = external dso_local constant { i8*, i8* }, align 8 +@_ZTI1Y = external dso_local constant { i8*, i8*, i8* }, align 8 + +define internal i8* @_ZN1Y3barEv(%struct.Y* %this) align 2 { +entry: + %0 = bitcast %struct.Y* %this to i8* + ret i8* %0 +} + +define dso_local i8* @_Z3fooP1X(%struct.X* %x) { +entry: + %0 = icmp eq %struct.X* %x, null + br i1 %0, label %dynamic_cast.null, label %dynamic_cast.notnull + +dynamic_cast.notnull: ; preds = %entry + %1 = bitcast %struct.X* %x to i8* + %2 = call i8* @__dynamic_cast(i8* %1, i8* bitcast ({ i8*, i8* }* @_ZTI1X to i8*), i8* bitcast ({ i8*, i8*, i8* }* @_ZTI1Y to i8*), i64 0) #2 + %3 = bitcast i8* %2 to %struct.Y* + br label %dynamic_cast.end + +dynamic_cast.null: ; preds = %entry + br label %dynamic_cast.end + +dynamic_cast.end: ; preds = %dynamic_cast.null, %dynamic_cast.notnull + %QQ5 = phi %struct.Y* [ %3, %dynamic_cast.notnull ], [ null, %dynamic_cast.null ] + %call = call i8* @_ZN1Y3barEv(%struct.Y* %QQ5) + ret i8* %call +} + +declare dso_local i8* @__dynamic_cast(i8*, i8*, i8*, i64) + +; UTC_ARGS: --enable + attributes #0 = { noinline nounwind uwtable } -- 2.7.4