From 1ad4527801e59d218615c5a73e2ac04904b49235 Mon Sep 17 00:00:00 2001 From: Matt Arsenault Date: Wed, 11 Jan 2023 09:57:21 -0500 Subject: [PATCH] Revert "llvm-reduce: Try to kill parallel workitems once we have a result." This reverts commit 4f575620d51032cf98424c9defafe4dfc8d66f45. I realized the test wasn't very good and when fixed, shows the reduction doesn't work correctly. Revert the change and keep the fixed version of the test. --- .../llvm-reduce/Inputs/sleep-and-check-stores.py | 28 +++++++++++++++ llvm/test/tools/llvm-reduce/Inputs/sleep.py | 8 ----- .../tools/llvm-reduce/parallel-workitem-kill.ll | 33 ++++-------------- llvm/tools/llvm-reduce/TestRunner.cpp | 40 +++++----------------- llvm/tools/llvm-reduce/TestRunner.h | 2 +- llvm/tools/llvm-reduce/deltas/Delta.cpp | 35 +++++++------------ llvm/tools/llvm-reduce/llvm-reduce.cpp | 5 ++- 7 files changed, 58 insertions(+), 93 deletions(-) create mode 100755 llvm/test/tools/llvm-reduce/Inputs/sleep-and-check-stores.py delete mode 100755 llvm/test/tools/llvm-reduce/Inputs/sleep.py diff --git a/llvm/test/tools/llvm-reduce/Inputs/sleep-and-check-stores.py b/llvm/test/tools/llvm-reduce/Inputs/sleep-and-check-stores.py new file mode 100755 index 0000000..8df093e --- /dev/null +++ b/llvm/test/tools/llvm-reduce/Inputs/sleep-and-check-stores.py @@ -0,0 +1,28 @@ +#!/bin/python + +import time +import sys + +sleep_seconds = int(sys.argv[1]) +num_stores = int(sys.argv[2]) +file_input = sys.argv[3] + +try: + input = open(file_input, "r") +except Exception as err: + print(err, file=sys.stderr) + sys.exit(1) + +InterestingStores = 0 +for line in input: + if "store" in line: + InterestingStores += 1 + +print("Interesting stores ", InterestingStores, " sleeping ", sleep_seconds) +time.sleep(sleep_seconds) + + +if InterestingStores > num_stores: + sys.exit(0) # interesting! + +sys.exit(1) # IR isn't interesting diff --git a/llvm/test/tools/llvm-reduce/Inputs/sleep.py b/llvm/test/tools/llvm-reduce/Inputs/sleep.py deleted file mode 100755 index 8fd230a..0000000 --- a/llvm/test/tools/llvm-reduce/Inputs/sleep.py +++ /dev/null @@ -1,8 +0,0 @@ -#!/bin/python - -import time -import sys - -sleep_seconds = int(sys.argv[1]) -time.sleep(sleep_seconds) - diff --git a/llvm/test/tools/llvm-reduce/parallel-workitem-kill.ll b/llvm/test/tools/llvm-reduce/parallel-workitem-kill.ll index 8003548..da52a8b 100644 --- a/llvm/test/tools/llvm-reduce/parallel-workitem-kill.ll +++ b/llvm/test/tools/llvm-reduce/parallel-workitem-kill.ll @@ -1,8 +1,14 @@ ; REQUIRES: thread_support -; RUN: llvm-reduce --process-poll-interval=1 -j 4 %s -o %t --delta-passes=instructions --test %python --test-arg %S/Inputs/sleep.py --test-arg 2 +; RUN: llvm-reduce -j 4 %s -o %t --delta-passes=instructions --test %python --test-arg %S/Inputs/sleep-and-check-stores.py --test-arg 1 --test-arg 5 ; RUN: FileCheck %s < %t ; CHECK: define void @foo +; CHECK: store +; CHECK: store +; CHECK: store +; CHECK: store +; CHECK: store +; CHECK: store ; CHECK-NEXT: ret void define void @foo(ptr %ptr) { @@ -23,30 +29,5 @@ define void @foo(ptr %ptr) { store i32 14, ptr %ptr store i32 15, ptr %ptr store i32 16, ptr %ptr - store i32 17, ptr %ptr - store i32 18, ptr %ptr - store i32 19, ptr %ptr - store i32 20, ptr %ptr - store i32 21, ptr %ptr - store i32 22, ptr %ptr - store i32 23, ptr %ptr - store i32 24, ptr %ptr - store i32 25, ptr %ptr - store i32 26, ptr %ptr - store i32 27, ptr %ptr - store i32 28, ptr %ptr - store i32 29, ptr %ptr - store i32 30, ptr %ptr - store i32 31, ptr %ptr - store i32 32, ptr %ptr - store i32 33, ptr %ptr - store i32 34, ptr %ptr - store i32 35, ptr %ptr - store i32 36, ptr %ptr - store i32 37, ptr %ptr - store i32 38, ptr %ptr - store i32 39, ptr %ptr - store i32 40, ptr %ptr ret void } - diff --git a/llvm/tools/llvm-reduce/TestRunner.cpp b/llvm/tools/llvm-reduce/TestRunner.cpp index 1ec761a..3a5483c 100644 --- a/llvm/tools/llvm-reduce/TestRunner.cpp +++ b/llvm/tools/llvm-reduce/TestRunner.cpp @@ -18,12 +18,6 @@ using namespace llvm; -extern cl::OptionCategory LLVMReduceOptions; -static cl::opt PollInterval("process-poll-interval", - cl::desc("child process wait polling"), - cl::init(5), cl::Hidden, - cl::cat(LLVMReduceOptions)); - TestRunner::TestRunner(StringRef TestName, const std::vector &TestArgs, std::unique_ptr Program, @@ -43,7 +37,7 @@ static constexpr std::array, 3> NullRedirects; /// Runs the interestingness test, passes file to be tested as first argument /// and other specified test arguments after that. -int TestRunner::run(StringRef Filename, const std::atomic &Killed) const { +int TestRunner::run(StringRef Filename) const { std::vector ProgramArgs; ProgramArgs.push_back(TestName); @@ -53,13 +47,13 @@ int TestRunner::run(StringRef Filename, const std::atomic &Killed) const { ProgramArgs.push_back(Filename); std::string ErrMsg; - bool ExecutionFailed; - sys::ProcessInfo PI = - sys::ExecuteNoWait(TestName, ProgramArgs, /*Env=*/std::nullopt, - Verbose ? DefaultRedirects : NullRedirects, - /*MemoryLimit=*/0, &ErrMsg, &ExecutionFailed); - if (ExecutionFailed) { + int Result = + sys::ExecuteAndWait(TestName, ProgramArgs, /*Env=*/std::nullopt, + Verbose ? DefaultRedirects : NullRedirects, + /*SecondsToWait=*/0, /*MemoryLimit=*/0, &ErrMsg); + + if (Result < 0) { Error E = make_error("Error running interesting-ness test: " + ErrMsg, inconvertibleErrorCode()); @@ -67,25 +61,7 @@ int TestRunner::run(StringRef Filename, const std::atomic &Killed) const { exit(1); } - // Poll every few seconds, taking a break to check if we should try to kill - // the process. We're trying to early exit on long running parallel reductions - // once we know they don't matter. - std::optional SecondsToWait(PollInterval); - bool Polling = true; - sys::ProcessInfo WaitPI; - - while (WaitPI.Pid == 0) { // Process has not changed state. - WaitPI = sys::Wait(PI, SecondsToWait, &ErrMsg, nullptr, Polling); - // TODO: This should probably be std::atomic_flag - if (Killed) { - // The current Program API does not have a way to directly kill, but we - // can timeout after 0 seconds. - SecondsToWait = 0; - Polling = false; - } - } - - return !WaitPI.ReturnCode; + return !Result; } void TestRunner::setProgram(std::unique_ptr P) { diff --git a/llvm/tools/llvm-reduce/TestRunner.h b/llvm/tools/llvm-reduce/TestRunner.h index 7af1096..128eede 100644 --- a/llvm/tools/llvm-reduce/TestRunner.h +++ b/llvm/tools/llvm-reduce/TestRunner.h @@ -33,7 +33,7 @@ public: /// Runs the interesting-ness test for the specified file /// @returns 0 if test was successful, 1 if otherwise - int run(StringRef Filename, const std::atomic &Killed) const; + int run(StringRef Filename) const; /// Returns the most reduced version of the original testcase ReducerWorkItem &getProgram() const { return *Program; } diff --git a/llvm/tools/llvm-reduce/deltas/Delta.cpp b/llvm/tools/llvm-reduce/deltas/Delta.cpp index 689d4fc..0def68c 100644 --- a/llvm/tools/llvm-reduce/deltas/Delta.cpp +++ b/llvm/tools/llvm-reduce/deltas/Delta.cpp @@ -65,8 +65,7 @@ void writeBitcode(ReducerWorkItem &M, raw_ostream &OutStream); void readBitcode(ReducerWorkItem &M, MemoryBufferRef Data, LLVMContext &Ctx, StringRef ToolName); -bool isReduced(ReducerWorkItem &M, const TestRunner &Test, - const std::atomic &Killed) { +bool isReduced(ReducerWorkItem &M, const TestRunner &Test) { const bool UseBitcode = Test.inputIsBitcode() || TmpFilesAsBitcode; SmallString<128> CurrentFilepath; @@ -97,7 +96,7 @@ bool isReduced(ReducerWorkItem &M, const TestRunner &Test, } // Current Chunks aren't interesting - return Test.run(CurrentFilepath, Killed); + return Test.run(CurrentFilepath); } /// Splits Chunks in half and prints them. @@ -139,8 +138,7 @@ CheckChunk(const Chunk &ChunkToCheckForUninterestingness, std::unique_ptr Clone, const TestRunner &Test, ReductionFunc ExtractChunksFromModule, const DenseSet &UninterestingChunks, - const std::vector &ChunksStillConsideredInteresting, - const std::atomic &Killed) { + const std::vector &ChunksStillConsideredInteresting) { // Take all of ChunksStillConsideredInteresting chunks, except those we've // already deemed uninteresting (UninterestingChunks) but didn't remove // from ChunksStillConsideredInteresting yet, and additionally ignore @@ -180,7 +178,7 @@ CheckChunk(const Chunk &ChunkToCheckForUninterestingness, errs() << "\n"; } - if (!isReduced(*Clone, Test, Killed)) { + if (!isReduced(*Clone, Test)) { // Program became non-reduced, so this chunk appears to be interesting. if (Verbose) errs() << "\n"; @@ -193,8 +191,7 @@ static SmallString<0> ProcessChunkFromSerializedBitcode( Chunk &ChunkToCheckForUninterestingness, TestRunner &Test, ReductionFunc ExtractChunksFromModule, DenseSet &UninterestingChunks, std::vector &ChunksStillConsideredInteresting, - SmallString<0> &OriginalBC, std::atomic &AnyReduced, - const std::atomic &Killed) { + SmallString<0> &OriginalBC, std::atomic &AnyReduced) { LLVMContext Ctx; auto CloneMMM = std::make_unique(); MemoryBufferRef Data(StringRef(OriginalBC), ""); @@ -204,7 +201,7 @@ static SmallString<0> ProcessChunkFromSerializedBitcode( if (std::unique_ptr ChunkResult = CheckChunk(ChunkToCheckForUninterestingness, std::move(CloneMMM), Test, ExtractChunksFromModule, UninterestingChunks, - ChunksStillConsideredInteresting, Killed)) { + ChunksStillConsideredInteresting)) { raw_svector_ostream BCOS(Result); writeBitcode(*ChunkResult, BCOS); // Communicate that the task reduced a chunk. @@ -245,7 +242,7 @@ void llvm::runDeltaPass(TestRunner &Test, ReductionFunc ExtractChunksFromModule, assert(!verifyReducerWorkItem(Test.getProgram(), &errs()) && "input module is broken after counting chunks"); - assert(isReduced(Test.getProgram(), Test, std::atomic()) && + assert(isReduced(Test.getProgram(), Test) && "input module no longer interesting after counting chunks"); #ifndef NDEBUG @@ -293,11 +290,6 @@ void llvm::runDeltaPass(TestRunner &Test, ReductionFunc ExtractChunksFromModule, writeBitcode(Test.getProgram(), BCOS); } - // If doing parallel reduction, signal to running workitem threads that we - // no longer care about their results. They should try to kill the reducer - // workitem process and exit. - std::atomic Killed = false; - SharedTaskQueue TaskQueue; for (auto I = ChunksStillConsideredInteresting.rbegin(), E = ChunksStillConsideredInteresting.rend(); @@ -324,12 +316,11 @@ void llvm::runDeltaPass(TestRunner &Test, ReductionFunc ExtractChunksFromModule, for (unsigned J = 0; J < NumInitialTasks; ++J) { TaskQueue.emplace_back(ChunkThreadPool.async( [J, I, &Test, &ExtractChunksFromModule, &UninterestingChunks, - &ChunksStillConsideredInteresting, &OriginalBC, &AnyReduced, - &Killed]() { + &ChunksStillConsideredInteresting, &OriginalBC, &AnyReduced]() { return ProcessChunkFromSerializedBitcode( *(I + J), Test, ExtractChunksFromModule, UninterestingChunks, ChunksStillConsideredInteresting, - OriginalBC, AnyReduced, Killed); + OriginalBC, AnyReduced); })); } @@ -353,11 +344,11 @@ void llvm::runDeltaPass(TestRunner &Test, ReductionFunc ExtractChunksFromModule, TaskQueue.emplace_back(ChunkThreadPool.async( [&Test, &ExtractChunksFromModule, &UninterestingChunks, &ChunksStillConsideredInteresting, &OriginalBC, - &ChunkToCheck, &AnyReduced, &Killed]() { + &ChunkToCheck, &AnyReduced]() { return ProcessChunkFromSerializedBitcode( ChunkToCheck, Test, ExtractChunksFromModule, UninterestingChunks, ChunksStillConsideredInteresting, - OriginalBC, AnyReduced, Killed); + OriginalBC, AnyReduced); })); } continue; @@ -370,8 +361,6 @@ void llvm::runDeltaPass(TestRunner &Test, ReductionFunc ExtractChunksFromModule, break; } - Killed = true; - // If we broke out of the loop, we still need to wait for everything to // avoid race access to the chunk set. // @@ -386,7 +375,7 @@ void llvm::runDeltaPass(TestRunner &Test, ReductionFunc ExtractChunksFromModule, *I, cloneReducerWorkItem(Test.getProgram(), Test.getTargetMachine()), Test, ExtractChunksFromModule, UninterestingChunks, - ChunksStillConsideredInteresting, Killed); + ChunksStillConsideredInteresting); } if (!Result) diff --git a/llvm/tools/llvm-reduce/llvm-reduce.cpp b/llvm/tools/llvm-reduce/llvm-reduce.cpp index 8f064bc..07a04a6 100644 --- a/llvm/tools/llvm-reduce/llvm-reduce.cpp +++ b/llvm/tools/llvm-reduce/llvm-reduce.cpp @@ -101,8 +101,7 @@ static cl::opt static codegen::RegisterCodeGenFlags CGF; -bool isReduced(ReducerWorkItem &M, const TestRunner &Test, - const std::atomic &Killed); +bool isReduced(ReducerWorkItem &M, const TestRunner &Test); /// Turn off crash debugging features /// @@ -218,7 +217,7 @@ int main(int Argc, char **Argv) { // test, rather than evaluating the source IR directly. This is for the // convenience of lit tests; the stripped out comments may have broken the // interestingness checks. - if (!isReduced(Tester.getProgram(), Tester, std::atomic())) { + if (!isReduced(Tester.getProgram(), Tester)) { errs() << "\nInput isn't interesting! Verify interesting-ness test\n"; return 1; } -- 2.7.4