From 7db2c64f61f777c68cffebfe9f78808e367132fa Mon Sep 17 00:00:00 2001 From: Pietro Fezzardi Date: Fri, 8 Oct 2021 11:51:31 +0200 Subject: [PATCH] MarkForSerialization now ignores duplicated uses Remove the logic for detecting Instructions with duplicated uses introduced by control-flow restructuring (the use is duplicated, but the instruction is not). By dropping this detection, we'll end up not marking for serialization some Instructions. Hence, when emitting C code, such Instructions will just be emitted as inline expressions, without declaring a dedicated local variable to hold their value. This is somehow suboptimal w.r.t the fact that the expression will be emitted many times, one for each duplicated use. However, this is not semantically incorrect, just verbose. On the other hand, the logic for detecting Instructions with duplicated uses has always been subtly broken, because it only looked at the number of duplicates for a given basic block introduced by control-flow restructuring. This information is not enough to detect Instructions with duplicated uses. Proper detection should actually be based on GHAST. --- .../MarkForSerialization/MarkAnalysis.h | 36 +++---------- .../MarkForSerializationFlags.h | 5 +- lib/BeautifyGHAST/BeautifyGHASTPass.cpp | 4 +- .../MarkForSerializationPass.cpp | 9 +--- tests/Unit/MarkForSerializationTest.cpp | 53 +------------------ 5 files changed, 12 insertions(+), 95 deletions(-) diff --git a/include/revng-c/MarkForSerialization/MarkAnalysis.h b/include/revng-c/MarkForSerialization/MarkAnalysis.h index 7e8df4654..061595218 100644 --- a/include/revng-c/MarkForSerialization/MarkAnalysis.h +++ b/include/revng-c/MarkForSerialization/MarkAnalysis.h @@ -190,8 +190,7 @@ public: using SuccVector = llvm::SmallVector; -template -class Analysis : public MonotoneFramework, +class Analysis : public MonotoneFramework, private: const llvm::Function &F; SerializationMap &ToSerialize; - const DuplicationMap &NDuplicates; LivenessAnalysis::LivenessMap LiveIn; public: @@ -216,18 +214,9 @@ public: revng_assert(A.lowerThanOrEqual(B)); } - Analysis(const llvm::Function &F, - const DuplicationMap &NDuplicates, - SerializationMap &ToSerialize) : - Base(&F.getEntryBlock()), - F(F), - ToSerialize(ToSerialize), - NDuplicates(NDuplicates), - LiveIn() { + Analysis(const llvm::Function &F, SerializationMap &ToSerialize) : + Base(&F.getEntryBlock()), F(F), ToSerialize(ToSerialize), LiveIn() { Base::registerExtremal(&F.getEntryBlock()); - if constexpr (IgnoreDuplicatedUses) { - revng_assert(NDuplicates.empty()); - } } [[noreturn]] void dumpFinalState() const { revng_abort(); } @@ -292,10 +281,6 @@ public: LatticeElement Pending = this->State[BB].copy(); - size_t NBBDuplicates = 0; - if constexpr (not IgnoreDuplicatedUses) - NBBDuplicates = NDuplicates.at(BB); - for (const Instruction &I : *BB) { revng_log(MarkLog, "Analyzing Instr: '" << &I << "': " << dumpToString(&I)); @@ -373,20 +358,13 @@ public: switch (I.getNumUses()) { case 1: { - if constexpr (not IgnoreDuplicatedUses) { - User *U = I.uses().begin()->getUser(); - Instruction *UserI = cast(U); - BasicBlock *UserBB = UserI->getParent(); - auto UserNDuplicates = NDuplicates.at(UserBB); - if (NBBDuplicates < UserNDuplicates) { - ToSerialize[&I].set(HasDuplicatedUses); - revng_log(MarkLog, "Instr HasDuplicatedUses"); - } - } + // Instructions with a single used do not necessarily need to be + // serialized. } break; case 0: { - // Do nothing + // Force unused instructions to be serialized. This is done to ease + // debugging, and could potentially be dropped in the future. ToSerialize[&I].set(AlwaysSerialize); revng_log(MarkLog, "Instr AlwaysSerialize"); } break; diff --git a/include/revng-c/MarkForSerialization/MarkForSerializationFlags.h b/include/revng-c/MarkForSerialization/MarkForSerializationFlags.h index 8f73f901e..bcce17c2d 100644 --- a/include/revng-c/MarkForSerialization/MarkForSerializationFlags.h +++ b/include/revng-c/MarkForSerialization/MarkForSerializationFlags.h @@ -17,9 +17,8 @@ enum SerializationReason { HasSideEffects = 1 << 1, HasInterferingSideEffects = 1 << 2, HasManyUses = 1 << 3, - HasDuplicatedUses = 1 << 4, - NeedsLocalVarToComputeExpr = 1 << 5, - NeedsManyStatements = 1 << 6, + NeedsLocalVarToComputeExpr = 1 << 4, + NeedsManyStatements = 1 << 5, }; inline SerializationReason diff --git a/lib/BeautifyGHAST/BeautifyGHASTPass.cpp b/lib/BeautifyGHAST/BeautifyGHASTPass.cpp index 4030fa643..d6caa4d2b 100644 --- a/lib/BeautifyGHAST/BeautifyGHASTPass.cpp +++ b/lib/BeautifyGHAST/BeautifyGHASTPass.cpp @@ -45,9 +45,7 @@ bool BeautifyGHASTPass::runOnFunction(llvm::Function &F) { // Get information about which instructions are marked to be serialized. // Mark instructions for serialization, and write the results in ToSerialize SerializationMap ToSerialize = {}; - MarkAnalysis::Analysis Mark(F, - {}, - ToSerialize); + MarkAnalysis::Analysis Mark(F, ToSerialize); Mark.initialize(); Mark.run(); diff --git a/lib/MarkForSerialization/MarkForSerializationPass.cpp b/lib/MarkForSerialization/MarkForSerializationPass.cpp index b1088046d..f696f2de5 100644 --- a/lib/MarkForSerialization/MarkForSerializationPass.cpp +++ b/lib/MarkForSerialization/MarkForSerializationPass.cpp @@ -39,16 +39,9 @@ bool MarkForSerializationPass::runOnFunction(llvm::Function &F) { if (not F.getName().equals(TargetFunction.c_str())) return false; - // Compute the number of duplicates for each BasicBlock. - const auto &RestructurePass = getAnalysis(); - using MarkAnalysis::DuplicationMap; - const DuplicationMap &NDuplicates = RestructurePass.getNDuplicates(); - // Mark instructions for serialization, and write the results in ToSerialize ToSerialize = {}; - MarkAnalysis::Analysis Mark(F, - NDuplicates, - ToSerialize); + MarkAnalysis::Analysis Mark(F, ToSerialize); Mark.initialize(); Mark.run(); diff --git a/tests/Unit/MarkForSerializationTest.cpp b/tests/Unit/MarkForSerializationTest.cpp index 75efda508..42b4558b7 100644 --- a/tests/Unit/MarkForSerializationTest.cpp +++ b/tests/Unit/MarkForSerializationTest.cpp @@ -49,17 +49,8 @@ runTestOnFunctionWithExpected(const char *Body, revng_check(nullptr != F); revng_check(not F->empty()); - // Initialize NDuplicates. Normally this is done by the llvm pass, but for - // testing reason we provide it manually. - MarkAnalysis::DuplicationMap NDuplicates; - revng_check(ExpectedDups.size() == F->size()); - for (const auto &[BB, NDup] : llvm::zip_first(*F, ExpectedDups)) - NDuplicates[&BB] = NDup; - SerializationMap Results; - MarkAnalysis::Analysis Mark(*F, - NDuplicates, - Results); + MarkAnalysis::Analysis Mark(*F, Results); Mark.initialize(); Mark.run(); @@ -228,48 +219,6 @@ BOOST_AUTO_TEST_CASE(BranchNone) { runTestOnFunctionWithExpected(Body, ExpectedDups, ExpectedFlags); } -BOOST_AUTO_TEST_CASE(Duplicated) { - - const char *Body = R"LLVM( - %pointer = inttoptr i64 4294967296 to i64* - br label %next - - next: - %loaded = load i64, i64 * %pointer - unreachable - )LLVM"; - - ExpectedDuplicatesType ExpectedDups{ 1, 2 }; - - ExpectedFlagsType ExpectedFlags{ - BBSerializationFlags{ "initial_block", { HasDuplicatedUses, None } }, - BBSerializationFlags{ "next", { AlwaysSerialize, AlwaysSerialize } }, - }; - - runTestOnFunctionWithExpected(Body, ExpectedDups, ExpectedFlags); -} - -BOOST_AUTO_TEST_CASE(DuplicatedDoesNotInduceVariable) { - - const char *Body = R"LLVM( - %pointer = inttoptr i64 4294967296 to i64* - br label %next - - next: - %loaded = load i64, i64 * %pointer - unreachable - )LLVM"; - - ExpectedDuplicatesType ExpectedDups{ 2, 2 }; - - ExpectedFlagsType ExpectedFlags{ - BBSerializationFlags{ "initial_block", { None, None } }, - BBSerializationFlags{ "next", { AlwaysSerialize, AlwaysSerialize } }, - }; - - runTestOnFunctionWithExpected(Body, ExpectedDups, ExpectedFlags); -} - BOOST_AUTO_TEST_CASE(Interfering) { const char *Body = R"LLVM(