[llvm] [MachineVerifier] Report errors from one thread at a time (PR #111605)
Ellis Hoag via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 11 10:39:38 PDT 2024
https://github.com/ellishg updated https://github.com/llvm/llvm-project/pull/111605
>From 47502221111a8d985b081dce67009ca3a0976738 Mon Sep 17 00:00:00 2001
From: Ellis Hoag <ellis.sparky.hoag at gmail.com>
Date: Tue, 8 Oct 2024 15:57:29 -0700
Subject: [PATCH 1/3] [MachineVerifier] Report errors from one thread at a time
---
llvm/lib/CodeGen/MachineVerifier.cpp | 105 +++++++++++-------
.../test_g_multiple_errors.mir | 22 ++++
2 files changed, 85 insertions(+), 42 deletions(-)
create mode 100644 llvm/test/MachineVerifier/test_g_multiple_errors.mir
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index b7218afdd38206..a39d364b1c02bd 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -77,8 +77,10 @@
#include "llvm/Pass.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/ManagedStatic.h"
#include "llvm/Support/MathExtras.h"
#include "llvm/Support/ModRef.h"
+#include "llvm/Support/Mutex.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/Target/TargetMachine.h"
#include <algorithm>
@@ -93,21 +95,29 @@ using namespace llvm;
namespace {
+static ManagedStatic<sys::SmartMutex<true>> ReportedErrorsLock;
+
struct MachineVerifier {
MachineVerifier(MachineFunctionAnalysisManager &MFAM, const char *b,
- raw_ostream *OS)
- : MFAM(&MFAM), OS(OS ? *OS : nulls()), Banner(b) {}
+ raw_ostream *OS, bool AbortOnError = true)
+ : MFAM(&MFAM), OS(OS ? *OS : nulls()), Banner(b),
+ ReportedErrs(AbortOnError) {}
- MachineVerifier(Pass *pass, const char *b, raw_ostream *OS)
- : PASS(pass), OS(OS ? *OS : nulls()), Banner(b) {}
+ MachineVerifier(Pass *pass, const char *b, raw_ostream *OS,
+ bool AbortOnError = true)
+ : PASS(pass), OS(OS ? *OS : nulls()), Banner(b),
+ ReportedErrs(AbortOnError) {}
MachineVerifier(const char *b, LiveVariables *LiveVars,
LiveIntervals *LiveInts, LiveStacks *LiveStks,
- SlotIndexes *Indexes, raw_ostream *OS)
+ SlotIndexes *Indexes, raw_ostream *OS,
+ bool AbortOnError = true)
: OS(OS ? *OS : nulls()), Banner(b), LiveVars(LiveVars),
- LiveInts(LiveInts), LiveStks(LiveStks), Indexes(Indexes) {}
+ LiveInts(LiveInts), LiveStks(LiveStks), Indexes(Indexes),
+ ReportedErrs(AbortOnError) {}
- unsigned verify(const MachineFunction &MF);
+ /// \returns true if no problems were found.
+ bool verify(const MachineFunction &MF);
MachineFunctionAnalysisManager *MFAM = nullptr;
Pass *const PASS = nullptr;
@@ -120,8 +130,6 @@ struct MachineVerifier {
const MachineRegisterInfo *MRI = nullptr;
const RegisterBankInfo *RBI = nullptr;
- unsigned foundErrors = 0;
-
// Avoid querying the MachineFunctionProperties for each operand.
bool isFunctionRegBankSelected = false;
bool isFunctionSelected = false;
@@ -231,6 +239,39 @@ struct MachineVerifier {
LiveStacks *LiveStks = nullptr;
SlotIndexes *Indexes = nullptr;
+ class ReportedErrors {
+ unsigned NumReported = 0;
+ bool AbortOnError;
+
+ public:
+ ReportedErrors(bool AbortOnError) : AbortOnError(AbortOnError) {}
+ ~ReportedErrors() {
+ if (!hasError())
+ return;
+ if (AbortOnError)
+ report_fatal_error("Found " + Twine(NumReported) +
+ " machine code errors.");
+ // Since we haven't aborted, release the lock to allow other threads to
+ // report errors.
+ ReportedErrorsLock->unlock();
+ }
+
+ /// Increment the number of reported errors.
+ /// \returns true if this is the first reported error.
+ bool increment() {
+ // If this is the first error this thread has encountered, grab the lock
+ // to prevent other threads from reporting errors at the same time.
+ // Otherwise we assume we already have the lock.
+ if (!hasError())
+ ReportedErrorsLock->lock();
+ ++NumReported;
+ return NumReported == 1;
+ }
+
+ bool hasError() { return NumReported; }
+ };
+ ReportedErrors ReportedErrs;
+
// This is calculated only when trying to verify convergence control tokens.
// Similar to the LLVM IR verifier, we calculate this locally instead of
// relying on the pass manager.
@@ -337,11 +378,7 @@ struct MachineVerifierLegacyPass : public MachineFunctionPass {
MachineFunctionProperties::Property::FailsVerification))
return false;
- unsigned FoundErrors =
- MachineVerifier(this, Banner.c_str(), &errs()).verify(MF);
- if (FoundErrors)
- report_fatal_error("Found " + Twine(FoundErrors) +
- " machine code errors.");
+ MachineVerifier(this, Banner.c_str(), &errs()).verify(MF);
return false;
}
};
@@ -357,10 +394,7 @@ MachineVerifierPass::run(MachineFunction &MF,
if (MF.getProperties().hasProperty(
MachineFunctionProperties::Property::FailsVerification))
return PreservedAnalyses::all();
- unsigned FoundErrors =
- MachineVerifier(MFAM, Banner.c_str(), &errs()).verify(MF);
- if (FoundErrors)
- report_fatal_error("Found " + Twine(FoundErrors) + " machine code errors.");
+ MachineVerifier(MFAM, Banner.c_str(), &errs()).verify(MF);
return PreservedAnalyses::all();
}
@@ -380,31 +414,20 @@ void llvm::verifyMachineFunction(const std::string &Banner,
// LiveIntervals *LiveInts;
// LiveStacks *LiveStks;
// SlotIndexes *Indexes;
- unsigned FoundErrors =
- MachineVerifier(nullptr, Banner.c_str(), &errs()).verify(MF);
- if (FoundErrors)
- report_fatal_error("Found " + Twine(FoundErrors) + " machine code errors.");
+ MachineVerifier(nullptr, Banner.c_str(), &errs()).verify(MF);
}
bool MachineFunction::verify(Pass *p, const char *Banner, raw_ostream *OS,
- bool AbortOnErrors) const {
- MachineFunction &MF = const_cast<MachineFunction&>(*this);
- unsigned FoundErrors = MachineVerifier(p, Banner, OS).verify(MF);
- if (AbortOnErrors && FoundErrors)
- report_fatal_error("Found "+Twine(FoundErrors)+" machine code errors.");
- return FoundErrors == 0;
+ bool AbortOnError) const {
+ return MachineVerifier(p, Banner, OS, AbortOnError).verify(*this);
}
bool MachineFunction::verify(LiveIntervals *LiveInts, SlotIndexes *Indexes,
const char *Banner, raw_ostream *OS,
- bool AbortOnErrors) const {
- MachineFunction &MF = const_cast<MachineFunction &>(*this);
- unsigned FoundErrors =
- MachineVerifier(Banner, nullptr, LiveInts, nullptr, Indexes, OS)
- .verify(MF);
- if (AbortOnErrors && FoundErrors)
- report_fatal_error("Found " + Twine(FoundErrors) + " machine code errors.");
- return FoundErrors == 0;
+ bool AbortOnError) const {
+ return MachineVerifier(Banner, /*LiveVars=*/nullptr, LiveInts,
+ /*LiveStks=*/nullptr, Indexes, OS, AbortOnError)
+ .verify(*this);
}
void MachineVerifier::verifySlotIndexes() const {
@@ -430,9 +453,7 @@ void MachineVerifier::verifyProperties(const MachineFunction &MF) {
report("Function has NoVRegs property but there are VReg operands", &MF);
}
-unsigned MachineVerifier::verify(const MachineFunction &MF) {
- foundErrors = 0;
-
+bool MachineVerifier::verify(const MachineFunction &MF) {
this->MF = &MF;
TM = &MF.getTarget();
TII = MF.getSubtarget().getInstrInfo();
@@ -447,7 +468,7 @@ unsigned MachineVerifier::verify(const MachineFunction &MF) {
// it's expected that the MIR is somewhat broken but that's ok since we'll
// reset it and clear the FailedISel attribute in ResetMachineFunctions.
if (isFunctionFailedISel)
- return foundErrors;
+ return true;
isFunctionRegBankSelected = MF.getProperties().hasProperty(
MachineFunctionProperties::Property::RegBankSelected);
@@ -544,13 +565,13 @@ unsigned MachineVerifier::verify(const MachineFunction &MF) {
regMasks.clear();
MBBInfoMap.clear();
- return foundErrors;
+ return !ReportedErrs.hasError();
}
void MachineVerifier::report(const char *msg, const MachineFunction *MF) {
assert(MF);
OS << '\n';
- if (!foundErrors++) {
+ if (ReportedErrs.increment()) {
if (Banner)
OS << "# " << Banner << '\n';
diff --git a/llvm/test/MachineVerifier/test_g_multiple_errors.mir b/llvm/test/MachineVerifier/test_g_multiple_errors.mir
new file mode 100644
index 00000000000000..0b44bef44e878b
--- /dev/null
+++ b/llvm/test/MachineVerifier/test_g_multiple_errors.mir
@@ -0,0 +1,22 @@
+# RUN: not --crash llc -mtriple=aarch64 -o /dev/null -run-pass=none -verify-machineinstrs %s 2>&1 | FileCheck %s --implicit-check-not="Bad machine code"
+# REQUIRES: aarch64-registered-target
+
+# Check that errors are reported from only one function.
+# CHECK: *** Bad machine code: Generic virtual register use cannot be undef ***
+# CHECK: Found 1 machine code errors.
+
+---
+name: foo
+liveins:
+body: |
+ bb.0:
+ $x0 = COPY undef %0:_(s64)
+...
+
+---
+name: bar
+liveins:
+body: |
+ bb.0:
+ $x0 = COPY undef %0:_(s64)
+...
>From b9ccb8bb4676658d09fc64792f39a818952ca7d1 Mon Sep 17 00:00:00 2001
From: Ellis Hoag <ellis.sparky.hoag at gmail.com>
Date: Wed, 9 Oct 2024 10:48:29 -0700
Subject: [PATCH 2/3] Fix test
---
.../{test_g_multiple_errors.mir => test_multiple_errors.mir} | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
rename llvm/test/MachineVerifier/{test_g_multiple_errors.mir => test_multiple_errors.mir} (71%)
diff --git a/llvm/test/MachineVerifier/test_g_multiple_errors.mir b/llvm/test/MachineVerifier/test_multiple_errors.mir
similarity index 71%
rename from llvm/test/MachineVerifier/test_g_multiple_errors.mir
rename to llvm/test/MachineVerifier/test_multiple_errors.mir
index 0b44bef44e878b..e1ce348565c1aa 100644
--- a/llvm/test/MachineVerifier/test_g_multiple_errors.mir
+++ b/llvm/test/MachineVerifier/test_multiple_errors.mir
@@ -1,7 +1,7 @@
-# RUN: not --crash llc -mtriple=aarch64 -o /dev/null -run-pass=none -verify-machineinstrs %s 2>&1 | FileCheck %s --implicit-check-not="Bad machine code"
+# RUN: not --crash llc -mtriple=aarch64 -o /dev/null -run-pass=none %s 2>&1 | FileCheck %s --implicit-check-not="Bad machine code"
# REQUIRES: aarch64-registered-target
-# Check that errors are reported from only one function.
+# Since we abort after reporting the first error, we should only expect one error to be reported.
# CHECK: *** Bad machine code: Generic virtual register use cannot be undef ***
# CHECK: Found 1 machine code errors.
>From 3d5fc043842d01a900b9c57b775b871bb7943af3 Mon Sep 17 00:00:00 2001
From: Ellis Hoag <ellis.sparky.hoag at gmail.com>
Date: Fri, 11 Oct 2024 10:39:27 -0700
Subject: [PATCH 3/3] add docs
---
llvm/lib/CodeGen/MachineVerifier.cpp | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index a39d364b1c02bd..e2c09fe25d55cd 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -95,6 +95,8 @@ using namespace llvm;
namespace {
+/// Used the by the ReportedErrors class to guarantee only one error is reported
+/// at one time.
static ManagedStatic<sys::SmartMutex<true>> ReportedErrorsLock;
struct MachineVerifier {
@@ -239,12 +241,16 @@ struct MachineVerifier {
LiveStacks *LiveStks = nullptr;
SlotIndexes *Indexes = nullptr;
+ /// A class to track the number of reported error and to guarantee that only
+ /// one error is reported at one time.
class ReportedErrors {
unsigned NumReported = 0;
bool AbortOnError;
public:
+ /// \param AbortOnError -- If set, abort after printing the first error.
ReportedErrors(bool AbortOnError) : AbortOnError(AbortOnError) {}
+
~ReportedErrors() {
if (!hasError())
return;
@@ -268,6 +274,7 @@ struct MachineVerifier {
return NumReported == 1;
}
+ /// \returns true if an error was reported.
bool hasError() { return NumReported; }
};
ReportedErrors ReportedErrs;
More information about the llvm-commits
mailing list