From cb57797550fe23f213b3793750a6bd63f08bee53 Mon Sep 17 00:00:00 2001 From: Scott Mansell Date: Wed, 8 Sep 2021 16:04:58 +1200 Subject: [PATCH 1/6] PassManager: Lookup pass by name --- .../FEXCore/Source/Interface/Core/Core.cpp | 14 +++++--- .../Source/Interface/Core/JIT/Arm64/JIT.cpp | 2 +- .../Source/Interface/Core/JIT/x86_64/JIT.cpp | 2 +- .../Source/Interface/IR/PassManager.cpp | 4 +-- .../FEXCore/Source/Interface/IR/PassManager.h | 35 ++++++++++++------- .../Interface/IR/Passes/IRValidation.cpp | 4 +-- 6 files changed, 39 insertions(+), 22 deletions(-) diff --git a/External/FEXCore/Source/Interface/Core/Core.cpp b/External/FEXCore/Source/Interface/Core/Core.cpp index 9c4da7179..995752c22 100644 --- a/External/FEXCore/Source/Interface/Core/Core.cpp +++ b/External/FEXCore/Source/Interface/Core/Core.cpp @@ -451,7 +451,13 @@ namespace FEXCore::Context { auto IRHandler = [Thread](uint64_t Addr, IR::IREmitter *IR) -> void { // Run the passmanager over the IR from the dispatcher Thread->PassManager->Run(IR); - Core::LocalIREntry Entry = {Addr, 0ULL, decltype(Entry.IR)(IR->CreateIRCopy()), decltype(Entry.RAData)(Thread->PassManager->GetRAPass() ? Thread->PassManager->GetRAPass()->PullAllocationData() : nullptr), decltype(Entry.DebugData)(new Core::DebugData())}; + Core::LocalIREntry Entry = {Addr, 0ULL, + decltype(Entry.IR)(IR->CreateIRCopy()), + decltype(Entry.RAData)(Thread->PassManager->HasPass("RA") + ? Thread->PassManager->GetPass("RA")->PullAllocationData() + : nullptr), + decltype(Entry.DebugData)(new Core::DebugData()) + }; Thread->LocalIRCache.insert({Addr, std::move(Entry)}); }; @@ -810,17 +816,17 @@ namespace FEXCore::Context { Thread->PassManager->Run(Thread->OpDispatcher.get()); if (Thread->CTX->Config.DumpIR() != "no") { - IRDumper(Thread->PassManager->GetRAPass() ? Thread->PassManager->GetRAPass()->GetAllocationData() : nullptr); + IRDumper(Thread->PassManager->HasPass("RA") ? Thread->PassManager->GetPass("RA")->GetAllocationData() : nullptr); } if (Thread->OpDispatcher->ShouldDump) { std::stringstream out; auto NewIR = Thread->OpDispatcher->ViewIR(); - FEXCore::IR::Dump(&out, &NewIR, Thread->PassManager->GetRAPass() ? Thread->PassManager->GetRAPass()->GetAllocationData() : nullptr); + FEXCore::IR::Dump(&out, &NewIR, Thread->PassManager->HasPass("RA") ? Thread->PassManager->GetPass("RA")->GetAllocationData() : nullptr); LogMan::Msg::I("IR 0x%lx:\n%s\n@@@@@\n", GuestRIP, out.str().c_str()); } - auto RAData = Thread->PassManager->GetRAPass() ? Thread->PassManager->GetRAPass()->PullAllocationData() : nullptr; + auto RAData = Thread->PassManager->HasPass("RA") ? Thread->PassManager->GetPass("RA")->PullAllocationData() : nullptr; auto IRList = Thread->OpDispatcher->CreateIRCopy(); Thread->OpDispatcher->ResetWorkingList(); diff --git a/External/FEXCore/Source/Interface/Core/JIT/Arm64/JIT.cpp b/External/FEXCore/Source/Interface/Core/JIT/Arm64/JIT.cpp index c9eb730f9..3c921120f 100644 --- a/External/FEXCore/Source/Interface/Core/JIT/Arm64/JIT.cpp +++ b/External/FEXCore/Source/Interface/Core/JIT/Arm64/JIT.cpp @@ -486,7 +486,7 @@ Arm64JITCore::Arm64JITCore(FEXCore::Context::Context *ctx, FEXCore::Core::Intern CurrentCodeBuffer = &InitialCodeBuffer; - RAPass = Thread->PassManager->GetRAPass(); + RAPass = Thread->PassManager->GetPass("RA"); #if DEBUG Decoder.AppendVisitor(&Disasm) diff --git a/External/FEXCore/Source/Interface/Core/JIT/x86_64/JIT.cpp b/External/FEXCore/Source/Interface/Core/JIT/x86_64/JIT.cpp index ba9f8b7c9..304c4e326 100644 --- a/External/FEXCore/Source/Interface/Core/JIT/x86_64/JIT.cpp +++ b/External/FEXCore/Source/Interface/Core/JIT/x86_64/JIT.cpp @@ -320,7 +320,7 @@ X86JITCore::X86JITCore(FEXCore::Context::Context *ctx, FEXCore::Core::InternalTh { CurrentCodeBuffer = &InitialCodeBuffer; - RAPass = Thread->PassManager->GetRAPass(); + RAPass = Thread->PassManager->GetPass("RA"); RAPass->AllocateRegisterSet(RegisterCount, RegisterClasses); RAPass->AddRegisters(FEXCore::IR::GPRClass, NumGPRs); diff --git a/External/FEXCore/Source/Interface/IR/PassManager.cpp b/External/FEXCore/Source/Interface/IR/PassManager.cpp index 8996c0dde..3baf16157 100644 --- a/External/FEXCore/Source/Interface/IR/PassManager.cpp +++ b/External/FEXCore/Source/Interface/IR/PassManager.cpp @@ -48,7 +48,7 @@ void PassManager::AddDefaultPasses(bool InlineConstants, bool StaticRegisterAllo // If the IR is compacted post-RA then the node indexing gets messed up and the backend isn't able to find the register assigned to a node // Compact before IR, don't worry about RA generating spills/fills - CompactionPass = InsertPass(CreateIRCompaction()); + InsertPass(CreateIRCompaction(), "Compaction"); } void PassManager::AddDefaultValidationPasses() { @@ -60,7 +60,7 @@ void PassManager::AddDefaultValidationPasses() { } void PassManager::InsertRegisterAllocationPass(bool OptimizeSRA) { - RAPass = InsertPass(IR::CreateRegisterAllocationPass(CompactionPass, OptimizeSRA)); + InsertPass(IR::CreateRegisterAllocationPass(GetPass("Compaction"), OptimizeSRA), "RA"); } bool PassManager::Run(IREmitter *IREmit) { diff --git a/External/FEXCore/Source/Interface/IR/PassManager.h b/External/FEXCore/Source/Interface/IR/PassManager.h index e0028705e..8815c4e15 100644 --- a/External/FEXCore/Source/Interface/IR/PassManager.h +++ b/External/FEXCore/Source/Interface/IR/PassManager.h @@ -20,7 +20,6 @@ class SyscallHandler; namespace FEXCore::IR { class PassManager; class IREmitter; -class RegisterAllocationPass; using ShouldExitHandler = std::function; @@ -42,9 +41,14 @@ class PassManager final { public: void AddDefaultPasses(bool InlineConstants, bool StaticRegisterAllocation); void AddDefaultValidationPasses(); - Pass* InsertPass(std::unique_ptr Pass) { + Pass* InsertPass(std::unique_ptr Pass, std::string Name = "") { Pass->RegisterPassManager(this); - return Passes.emplace_back(std::move(Pass)).get(); + auto PassPtr = Passes.emplace_back(std::move(Pass)).get(); + + if (!Name.empty()) { + NameToPassMaping[Name] = PassPtr; + } + return PassPtr; } void InsertRegisterAllocationPass(bool OptimizeSRA); @@ -55,12 +59,17 @@ public: ExitHandler = std::move(Handler); } - bool HasRAPass() const { - return RAPass != nullptr; + bool HasPass(std::string Name) const { + return NameToPassMaping.contains(Name); } - IR::RegisterAllocationPass *GetRAPass() { - return reinterpret_cast(RAPass); + template + T* GetPass(std::string Name) { + return dynamic_cast(NameToPassMaping[Name]); + } + + Pass* GetPass(std::string Name) { + return NameToPassMaping[Name]; } void RegisterSyscallHandler(FEXCore::HLE::SyscallHandler *Handler) { @@ -72,16 +81,18 @@ protected: FEXCore::HLE::SyscallHandler *SyscallHandler; private: - Pass *RAPass{}; - Pass *CompactionPass{}; - std::vector> Passes; + std::unordered_map NameToPassMaping; #if defined(ASSERTIONS_ENABLED) && ASSERTIONS_ENABLED std::vector> ValidationPasses; - void InsertValidationPass(std::unique_ptr Pass) { + void InsertValidationPass(std::unique_ptr Pass, std::string Name = "") { Pass->RegisterPassManager(this); - ValidationPasses.emplace_back(std::move(Pass)); + auto PassPtr = ValidationPasses.emplace_back(std::move(Pass)).get(); + + if (!Name.empty()) { + NameToPassMaping[Name] = PassPtr; + } } #endif diff --git a/External/FEXCore/Source/Interface/IR/Passes/IRValidation.cpp b/External/FEXCore/Source/Interface/IR/Passes/IRValidation.cpp index c867386f8..0333a1b1d 100644 --- a/External/FEXCore/Source/Interface/IR/Passes/IRValidation.cpp +++ b/External/FEXCore/Source/Interface/IR/Passes/IRValidation.cpp @@ -71,8 +71,8 @@ bool IRValidation::Run(IREmitter *IREmit) { #endif IR::RegisterAllocationData * RAData{}; - if (Manager->HasRAPass()) { - RAData = Manager->GetRAPass() ? Manager->GetRAPass()->GetAllocationData() : nullptr; + if (Manager->HasPass("RA")) { + RAData = Manager->GetPass("RA")->GetAllocationData(); } NodeIsLive.Set(1); // IRHEADER From 55a9ee702b8b4d026ae70ca055df805c0bcd298c Mon Sep 17 00:00:00 2001 From: Scott Mansell Date: Fri, 3 Sep 2021 11:15:08 +1200 Subject: [PATCH 2/6] Register Allocator Validation This is a validation pass that attempts to prove the output from the RA pass is valid. The current design should be able to prove that the RA result is internally consistent. That no-matter what control flow path you take thought the control flow graph, the physical registers and spill slots will always contain a single possible SSA value. It also checks that the SSA values in the IR actually line up with the SSA value in the physical register. --- External/FEXCore/Source/CMakeLists.txt | 1 + .../Source/Interface/IR/PassManager.cpp | 3 +- External/FEXCore/Source/Interface/IR/Passes.h | 1 + .../Interface/IR/Passes/IRValidation.cpp | 38 +- .../Source/Interface/IR/Passes/IRValidation.h | 32 ++ .../Interface/IR/Passes/RAValidation.cpp | 365 ++++++++++++++++++ 6 files changed, 417 insertions(+), 23 deletions(-) create mode 100644 External/FEXCore/Source/Interface/IR/Passes/IRValidation.h create mode 100644 External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp diff --git a/External/FEXCore/Source/CMakeLists.txt b/External/FEXCore/Source/CMakeLists.txt index eb1190e4a..560c6feb9 100644 --- a/External/FEXCore/Source/CMakeLists.txt +++ b/External/FEXCore/Source/CMakeLists.txt @@ -119,6 +119,7 @@ set (SRCS Interface/IR/Passes/DeadContextStoreElimination.cpp Interface/IR/Passes/IRCompaction.cpp Interface/IR/Passes/IRValidation.cpp + Interface/IR/Passes/RAValidation.cpp Interface/IR/Passes/LongDivideRemovalPass.cpp Interface/IR/Passes/ValueDominanceValidation.cpp Interface/IR/Passes/PhiValidation.cpp diff --git a/External/FEXCore/Source/Interface/IR/PassManager.cpp b/External/FEXCore/Source/Interface/IR/PassManager.cpp index 3baf16157..8cfaaf01c 100644 --- a/External/FEXCore/Source/Interface/IR/PassManager.cpp +++ b/External/FEXCore/Source/Interface/IR/PassManager.cpp @@ -54,7 +54,8 @@ void PassManager::AddDefaultPasses(bool InlineConstants, bool StaticRegisterAllo void PassManager::AddDefaultValidationPasses() { #if defined(ASSERTIONS_ENABLED) && ASSERTIONS_ENABLED InsertValidationPass(Validation::CreatePhiValidation()); - InsertValidationPass(Validation::CreateIRValidation()); + InsertValidationPass(Validation::CreateIRValidation(), "IRValidation"); + InsertValidationPass(Validation::CreateRAValidation()); InsertValidationPass(Validation::CreateValueDominanceValidation()); #endif } diff --git a/External/FEXCore/Source/Interface/IR/Passes.h b/External/FEXCore/Source/Interface/IR/Passes.h index f6731236c..4f026a95a 100644 --- a/External/FEXCore/Source/Interface/IR/Passes.h +++ b/External/FEXCore/Source/Interface/IR/Passes.h @@ -20,6 +20,7 @@ std::unique_ptr CreateLongDivideEliminationPass(); namespace Validation { std::unique_ptr CreateIRValidation(); +std::unique_ptr CreateRAValidation(); std::unique_ptr CreatePhiValidation(); std::unique_ptr CreateValueDominanceValidation(); } diff --git a/External/FEXCore/Source/Interface/IR/Passes/IRValidation.cpp b/External/FEXCore/Source/Interface/IR/Passes/IRValidation.cpp index 0333a1b1d..16c44bf38 100644 --- a/External/FEXCore/Source/Interface/IR/Passes/IRValidation.cpp +++ b/External/FEXCore/Source/Interface/IR/Passes/IRValidation.cpp @@ -6,8 +6,8 @@ $end_info$ */ #include "Interface/IR/PassManager.h" +#include "Interface/IR/Passes/IRValidation.h" #include "Interface/IR/Passes/RegisterAllocationPass.h" -#include "Common/BitSet.h" #include #include @@ -24,26 +24,8 @@ $end_info$ #include #include -namespace { - struct BlockInfo { - bool HasExit; - - std::vector Predecessors; - std::vector Successors; - }; -} - namespace FEXCore::IR::Validation { -class IRValidation final : public FEXCore::IR::Pass { -public: - ~IRValidation(); - bool Run(IREmitter *IREmit) override; - -private: - BitSet NodeIsLive; - size_t MaxNodes{}; -}; IRValidation::~IRValidation() { NodeIsLive.Free(); @@ -53,12 +35,14 @@ bool IRValidation::Run(IREmitter *IREmit) { bool HadError = false; bool HadWarning = false; - std::unordered_map OffsetToBlockMap; - auto CurrentIR = IREmit->ViewIR(); - std::ostringstream Errors; std::ostringstream Warnings; + auto CurrentIR = IREmit->ViewIR(); + + OffsetToBlockMap.clear(); + EntryBlock = nullptr; + if (CurrentIR.GetSSACount() > MaxNodes) { NodeIsLive.Realloc(CurrentIR.GetSSACount()); } @@ -81,10 +65,15 @@ bool IRValidation::Run(IREmitter *IREmit) { auto BlockIROp = BlockHeader->CW(); LOGMAN_THROW_A_FMT(BlockIROp->Header.Op == OP_CODEBLOCK, "IR type failed to be a code block"); + if (!EntryBlock) { + EntryBlock = BlockNode; + } + uint32_t BlockID = CurrentIR.GetID(BlockNode); BlockInfo *CurrentBlock = &OffsetToBlockMap.try_emplace(BlockID).first->second; + for (auto [CodeNode, IROp] : CurrentIR.GetCode(BlockNode)) { uint32_t ID = CurrentIR.GetID(CodeNode); @@ -292,6 +281,11 @@ bool IRValidation::Run(IREmitter *IREmit) { } LogMan::Msg::EFmt("{}", Out.str()); + + LOGMAN_MSG_A("Encountered IR validation Error"); + + Errors.clear(); + Warnings.clear(); } return false; diff --git a/External/FEXCore/Source/Interface/IR/Passes/IRValidation.h b/External/FEXCore/Source/Interface/IR/Passes/IRValidation.h new file mode 100644 index 000000000..7c3ee4d0e --- /dev/null +++ b/External/FEXCore/Source/Interface/IR/Passes/IRValidation.h @@ -0,0 +1,32 @@ +#pragma once + +#include "Common/BitSet.h" +#include + +namespace FEXCore::IR::Validation { + +struct BlockInfo { + bool HasExit; + OrderedNode const *BlockNode; + + std::vector Predecessors; + std::vector Successors; +}; + +class RAValidation; + +class IRValidation final : public FEXCore::IR::Pass { +public: + ~IRValidation(); + bool Run(IREmitter *IREmit) override; + +private: + + BitSet NodeIsLive; + OrderedNode *EntryBlock; + std::unordered_map OffsetToBlockMap; + size_t MaxNodes{}; + + friend class RAValidation; +}; +} diff --git a/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp b/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp new file mode 100644 index 000000000..3cc9711b4 --- /dev/null +++ b/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp @@ -0,0 +1,365 @@ +#include "Interface/IR/PassManager.h" +#include "Interface/IR/Passes/IRValidation.h" +#include "Interface/IR/Passes/RegisterAllocationPass.h" + +#include +#include +#include +#include + + +#include +#include +#include + +namespace FEXCore::IR::Validation { + +struct RegState { + static constexpr uint32_t UninitializedValue = 0; + static constexpr uint32_t InvalidReg = 0xffff'ffff; + static constexpr uint32_t CorruptedPair = 0xffff'fffe; + static constexpr uint32_t ClobberedValue = 0xffff'fffd; + static constexpr uint32_t StaticAssigned = 0xffff'ff00; + + // This class makes some assumptions about how the host registers are arranged and mapped to virtual registers: + // 1. There will be less than 32 GPRs and 32 FPRs + // 2. If the GPRFixed class is used, there will be 16 GPRs and 16 FixedGPRs max + // 3. Same with FPRFixed + // 4. If the GPRPairClass is used, it is assumed each GPRPair N will map onto GPRs N*2 and N*2 + 1 + + // These assumptions were all true for the state of the arm64 and x86 jits at the time this was written + + bool Set(PhysicalRegister Reg, uint32_t ssa) { + LOGMAN_THROW_A(ssa != 0, "RegState assumes ssa0 will be the block header and never assigned to a register"); + + switch (Reg.Class) { + case GPRClass: + GPRs[Reg.Reg] = ssa; + return true; + case GPRFixedClass: + // On arm64, there are 16 Fixed and 9 normal + GPRs[Reg.Reg + 16] = ssa; + return true; + case FPRClass: + FPRs[Reg.Reg] = ssa; + return true; + case FPRFixedClass: + // On arm64, there are 16 Fixed and 12 normal + FPRs[Reg.Reg + 16] = ssa; + return true; + case GPRPairClass: + if (Reg.Reg <= 16) { + // Alias paired registers onto both + GPRs[Reg.Reg*2] = ssa; + GPRs[Reg.Reg*2 + 1] = ssa; + return true; + } + break; + } + return false; + } + + uint32_t Get(PhysicalRegister Reg) { + switch (Reg.Class) { + case GPRClass: + return GPRs[Reg.Reg]; + case GPRFixedClass: + if (GPRs[Reg.Reg + 16] == UninitializedValue) { + return StaticAssigned; + } + return GPRs[Reg.Reg + 16]; + case FPRClass: + return FPRs[Reg.Reg]; + case FPRFixedClass: + if (FPRs[Reg.Reg + 16] == UninitializedValue) { + return StaticAssigned; + } + return FPRs[Reg.Reg + 16]; + case GPRPairClass: + if (Reg.Reg > 16) + break; + + // Make sure both halves of the Pair contain the same SSA + if (GPRs[Reg.Reg*2] == GPRs[Reg.Reg*2 + 1]) { + return GPRs[Reg.Reg*2]; + } + return CorruptedPair; + } + return InvalidReg; + } + + void Intersect(RegState& other) { + for (size_t i = 0; i < GPRs.size(); i++) { + if (GPRs[i] != other.GPRs[i]) { + GPRs[i] = ClobberedValue; + } + } + + for (size_t i = 0; i < FPRs.size(); i++) { + if (FPRs[i] != other.FPRs[i]) { + FPRs[i] = ClobberedValue; + } + } + } + + void Filter(uint32_t MaxSSA) { + for (auto &gpr : GPRs) { + if (gpr > MaxSSA) { + gpr = ClobberedValue; + } + } + + for (auto &fpr : FPRs) { + if (fpr > MaxSSA) { + fpr = ClobberedValue; + } + } + } + +private: + std::array GPRs = {}; + std::array FPRs = {}; + +public: + uint32_t Version{}; +}; + +class RAValidation final : public FEXCore::IR::Pass { +public: + ~RAValidation() {} + bool Run(IREmitter *IREmit) override; + +private: + std::unordered_map BlockExitState; + std::deque BlocksToVisit; +}; + + +bool RAValidation::Run(IREmitter *IREmit) { + if (!Manager->HasPass("RA")) return false; + + IR::RegisterAllocationData* RAData = Manager->GetPass("RA")->GetAllocationData(); + BlockExitState.clear(); + + // Get the control flow graph from the validation pass + auto ValidationPass = Manager->GetPass("IRValidation"); + LOGMAN_THROW_A(ValidationPass != nullptr, "Couldn't find IRValidation pass"); + + auto& OffsetToBlockMap = ValidationPass->OffsetToBlockMap; + + LOGMAN_THROW_A(ValidationPass->EntryBlock != nullptr, "No entry point"); + BlocksToVisit.push_front(ValidationPass->EntryBlock); // Currently only a single entry point + + bool HadError = false; + std::ostringstream Errors; + + auto CurrentIR = IREmit->ViewIR(); + uint32_t CurrentVersion = 1; // Incremented every backwards edge + + while (!BlocksToVisit.empty()) + { + auto BlockNode = BlocksToVisit.front(); + uint32_t BlockID = CurrentIR.GetID(BlockNode); + auto& BlockInfo = OffsetToBlockMap[BlockID]; + + auto IsFowardsEdge = [&] (uint32_t PredecessorID) { + // TODO: This isn't the best definition of fowards/backwards edges. It's possible for + // Blocks to be out of order. Will need a proper CFG analysis pass. + // + // But I don't think we currently generate Backwards branches that don't follow this rule + return PredecessorID < BlockID; + }; + + // First, make sure we have the exit state data for all Predecessor + bool MissingPredecessor = false; + + for (auto Predecessor : BlockInfo.Predecessors) { + auto PredecessorID = CurrentIR.GetID(Predecessor); + bool HaveState = BlockExitState.contains(PredecessorID) && BlockExitState[PredecessorID].Version == CurrentVersion; + + if (IsFowardsEdge(PredecessorID) && !HaveState) { + // We are probably about to visit this node anyway, remove it + std::remove(BlocksToVisit.begin(), BlocksToVisit.end(), Predecessor); + + // Add the missing predecessor to start of queue + BlocksToVisit.push_front(Predecessor); + MissingPredecessor = true; + } + } + + if (MissingPredecessor) { + // We'll have to come back to this block later + continue; + } + + // Remove block from queue + BlocksToVisit.pop_front(); + + bool FirstVisit = !BlockExitState.contains(BlockID); + + // Second, we need to determine the register status as of Block entry + auto BlockOp = CurrentIR.GetOp(BlockNode); + uint32_t FirstSSA = BlockOp->Begin.ID(); + + auto& BlockRegState = BlockExitState.try_emplace(BlockID).first->second; + bool EmptyRegState = true; + auto Intersect = [&] (RegState& Other) { + if (EmptyRegState) { + BlockRegState = Other; + EmptyRegState = false; + } else { + BlockRegState.Intersect(Other); + } + }; + + for (auto Predecessor : BlockInfo.Predecessors) { + auto PredecessorID = CurrentIR.GetID(Predecessor); + if (BlockExitState.contains(PredecessorID)) { + if (IsFowardsEdge(PredecessorID)) { + Intersect(BlockExitState[PredecessorID]); + } else { + RegState Filtered = BlockExitState[PredecessorID]; + Filtered.Filter(FirstSSA); + Intersect(Filtered); + } + } + } + + // Thrid, we need to iterate over all IR ops in the block + + for (auto [CodeNode, IROp] : CurrentIR.GetCode(BlockNode)) { + uint32_t ID = CurrentIR.GetID(CodeNode); + + // And check that all args point at the correct SSA + uint8_t NumArgs = IR::GetArgs(IROp->Op); + for (uint32_t i = 0; i < NumArgs; ++i) { + OrderedNodeWrapper Arg = IROp->Args[i]; + + const auto PhyReg = RAData->GetNodeRegister(Arg.ID()); + + if (PhyReg.IsInvalid()) + continue; + + auto CurrentSSAAtReg = BlockRegState.Get(PhyReg); + if (CurrentSSAAtReg == RegState::InvalidReg) { + HadError |= true; + Errors << fmt::format("%ssa{}: Arg[{}] unknown Reg: {}, class: {}\n", ID, i, PhyReg.Reg, PhyReg.Class); + } else if (CurrentSSAAtReg == RegState::CorruptedPair) { + HadError |= true; + + auto Lower = BlockRegState.Get(PhysicalRegister(GPRClass, uint8_t(PhyReg.Reg*2) + 1)); + auto Upper = BlockRegState.Get(PhysicalRegister(GPRClass, PhyReg.Reg*2 + 1)); + + Errors << fmt::format("%ssa{}: Arg[{}] expects paired reg{} to contain %ssa{}, but it actually contains {{%ssa{}, %ssa{}}}\n", + ID, i, PhyReg.Reg, Arg.ID(), Lower, Upper); + } else if (CurrentSSAAtReg == RegState::UninitializedValue) { + HadError |= true; + + Errors << fmt::format("%ssa{}: Arg[{}] expects reg{} to contain %ssa{}, but it is uninitialized\n", + ID, i, PhyReg.Reg, Arg.ID()); + } else if (CurrentSSAAtReg == RegState::ClobberedValue) { + HadError |= true; + + Errors << fmt::format("%ssa{}: Arg[{}] expects reg{} to contain %ssa{}, but contents vary depending on control flow\n", + ID, i, PhyReg.Reg, Arg.ID()); + } else if (CurrentSSAAtReg != Arg.ID()) { + HadError |= true; + Errors << fmt::format("%ssa{}: Arg[{}] expects reg{} to contain %ssa{}, but it actually contains %ssa{}\n", + ID, i, PhyReg.Reg, Arg.ID(), CurrentSSAAtReg); + } + } + + // Update BlockState map + BlockRegState.Set(RAData->GetNodeRegister(ID), ID); + } + + // Forth, Add successors to the queue of blocks to validate + for (auto Successor : BlockInfo.Successors) { + auto SuccessorID = CurrentIR.GetID(Successor); + + // TODO: This isn't the best definition of fowards/backwards edges. It's possible for + // Blocks to be out of order. Will need a proper CFG analysis pass. + // + // But I don't think we currently generate Backwards branches that don't follow this rule + bool FowardsEdge = SuccessorID > BlockID; + + if (FowardsEdge) { + // Always follow forwards edges, assuming it's not already on the queue + if (std::find(BlocksToVisit.begin(), BlocksToVisit.end(), Successor) == std::end(BlocksToVisit)) { + // Push to the back of queue so there is a higher chance all predecessors for this block are done first + BlocksToVisit.push_back(Successor); + } + } else if (FirstVisit) { + // Now that we have the block data for the backwards edge, we can visit it again and make + // sure it (and all it's successors) are still valid. + + // But only do this the first time we encounter each backwards edge. + + // Push to the front of queue, so we get this re-checking done before examining future nodes. + BlocksToVisit.push_front(Successor); + + // Make sure states are reprocessed + CurrentVersion++; + } + } + + BlockRegState.Version = CurrentVersion; + + if (CurrentVersion > 10000) { + Errors << "Infinite Loop\n"; + HadError |= true; + + for (auto [BlockNode, BlockHeader] : CurrentIR.GetBlocks()) { + uint32_t BlockID = CurrentIR.GetID(BlockNode); + auto& BlockInfo = OffsetToBlockMap[BlockID]; + + Errors << fmt::format("Block {}\n\tPredecessors: ", BlockID); + + for (auto Predecessor : BlockInfo.Predecessors) { + auto PredecessorID = CurrentIR.GetID(Predecessor); + bool FowardsEdge = PredecessorID < BlockID; + if (!FowardsEdge) { + Errors << "(Backwards): "; + } + Errors << fmt::format("Block {} ", PredecessorID); + } + + Errors << "\n\tSuccessors: "; + + for (auto Successor : BlockInfo.Successors) { + auto SuccessorID = CurrentIR.GetID(Successor); + bool FowardsEdge = SuccessorID > BlockID; + + if (!FowardsEdge) { + Errors << "(Backwards): "; + } + Errors << fmt::format("Block {} ", SuccessorID); + + } + + Errors << "\n\n"; + } + + break; + } + + } + + if (HadError) { + std::stringstream IrDump; + FEXCore::IR::Dump(&IrDump, &CurrentIR, RAData); + + LogMan::Msg::EFmt("RA Validation Error\n{}\nErrors:\n{}\n", IrDump.str(), Errors.str()); + + LOGMAN_MSG_A("Encountered RA validation Error"); + + Errors.clear(); + } + + return false; +} + +std::unique_ptr CreateRAValidation() { + return std::make_unique(); +} +} From 2cd844bbcb798a8aa779cef433bba284ca9fd71b Mon Sep 17 00:00:00 2001 From: Scott Mansell Date: Fri, 10 Sep 2021 07:22:57 +1200 Subject: [PATCH 3/6] RAValidation: Validate Spill slots This can prove that the ssa in the Spill slots are consistant based on control flow. It's one major blind spot is that it can't prove the spill slot actually contains the correct ssa, if it has the same wrong ssa on every cfg path. --- .../Interface/IR/Passes/RAValidation.cpp | 79 +++++++++++++++++-- 1 file changed, 73 insertions(+), 6 deletions(-) diff --git a/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp b/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp index 3cc9711b4..a669382b0 100644 --- a/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp +++ b/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp @@ -88,6 +88,20 @@ struct RegState { return InvalidReg; } + + void Spill(uint32_t SpillSlot, uint32_t ssa) { + Spills[SpillSlot] = ssa; + } + + uint32_t Unspill(uint32_t SpillSlot) { + if (Spills.contains(SpillSlot)) { + uint32_t Value = Spills[SpillSlot]; + Spills.erase(SpillSlot); + return Value; + } + return UninitializedValue; + } + void Intersect(RegState& other) { for (size_t i = 0; i < GPRs.size(); i++) { if (GPRs[i] != other.GPRs[i]) { @@ -100,6 +114,15 @@ struct RegState { FPRs[i] = ClobberedValue; } } + + for (auto it = Spills.begin(); it != Spills.end(); it++) { + auto& [SlotID, Value] = *it; + if (!other.Spills.contains(SlotID)) { + Spills.erase(it); + } else if (Value != other.Spills[SlotID]) { + Value = ClobberedValue; + } + } } void Filter(uint32_t MaxSSA) { @@ -114,12 +137,21 @@ struct RegState { fpr = ClobberedValue; } } + + for (auto it = Spills.begin(); it != Spills.end(); it++) { + auto& [SlotID, Value] = *it; + if (Value > MaxSSA) { + Spills.erase(it); + } + } } private: std::array GPRs = {}; std::array FPRs = {}; + std::unordered_map Spills; + public: uint32_t Version{}; }; @@ -230,15 +262,11 @@ bool RAValidation::Run(IREmitter *IREmit) { for (auto [CodeNode, IROp] : CurrentIR.GetCode(BlockNode)) { uint32_t ID = CurrentIR.GetID(CodeNode); - // And check that all args point at the correct SSA - uint8_t NumArgs = IR::GetArgs(IROp->Op); - for (uint32_t i = 0; i < NumArgs; ++i) { - OrderedNodeWrapper Arg = IROp->Args[i]; - + auto CheckArg = [&] (uint32_t i, OrderedNodeWrapper Arg) { const auto PhyReg = RAData->GetNodeRegister(Arg.ID()); if (PhyReg.IsInvalid()) - continue; + return; auto CurrentSSAAtReg = BlockRegState.Get(PhyReg); if (CurrentSSAAtReg == RegState::InvalidReg) { @@ -267,6 +295,45 @@ bool RAValidation::Run(IREmitter *IREmit) { Errors << fmt::format("%ssa{}: Arg[{}] expects reg{} to contain %ssa{}, but it actually contains %ssa{}\n", ID, i, PhyReg.Reg, Arg.ID(), CurrentSSAAtReg); } + }; + + switch (IROp->Op) + { + case OP_SPILLREGISTER: { + auto SpillRegister = IROp->C(); + CheckArg(0, SpillRegister->Value); + + BlockRegState.Spill(SpillRegister->Slot, SpillRegister->Value.ID()); + break; + } + + case OP_FILLREGISTER: { + auto FillRegister = IROp->C(); + uint32_t value = BlockRegState.Unspill(FillRegister->Slot); + + // TODO: This only proves that the Spill has a consistent SSA value + // In the future we need to prove it contains the correct SSA value + + if (value == RegState::UninitializedValue) { + HadError |= true; + Errors << fmt::format("%ssa{}: FillRegister; Spill Slot {} was undefined in at least one control flow path\n", + ID, FillRegister->Slot); + } else if (value == RegState::ClobberedValue) { + HadError |= true; + Errors << fmt::format("%ssa{}: FillRegister; Spill Slot {} contents vary depending on control flow\n", + ID, FillRegister->Slot); + } + break; + } + + default: { + // And check that all args point at the correct SSA + uint8_t NumArgs = IR::GetArgs(IROp->Op); + for (uint32_t i = 0; i < NumArgs; ++i) { + CheckArg(i, IROp->Args[i]); + } + break; + } } // Update BlockState map From 3107898f06c3cd40b09dbbe7b664bb2d8d717949 Mon Sep 17 00:00:00 2001 From: Scott Mansell Date: Mon, 13 Sep 2021 15:13:27 +1200 Subject: [PATCH 4/6] Fix ReplaceUsesWithAfter: There were two issues: 1. The OrderedNode *After overload created the wrong iterator 2. Despite being named After, they were actually inclusive Kept ReplaceAllUsesWithRange with the current inclusive behaviour and adjusted the argument name to match --- External/FEXCore/Source/Interface/IR/IREmitter.cpp | 8 ++++---- External/FEXCore/include/FEXCore/IR/IREmitter.h | 7 ++++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/External/FEXCore/Source/Interface/IR/IREmitter.cpp b/External/FEXCore/Source/Interface/IR/IREmitter.cpp index 5fd02015c..144b18ad0 100644 --- a/External/FEXCore/Source/Interface/IR/IREmitter.cpp +++ b/External/FEXCore/Source/Interface/IR/IREmitter.cpp @@ -26,12 +26,12 @@ void IREmitter::ResetWorkingList() { CurrentCodeBlock = nullptr; } -void IREmitter::ReplaceAllUsesWithRange(OrderedNode *Node, OrderedNode *NewNode, AllNodesIterator After, AllNodesIterator End) { +void IREmitter::ReplaceAllUsesWithRange(OrderedNode *Node, OrderedNode *NewNode, AllNodesIterator Begin, AllNodesIterator End) { uintptr_t ListBegin = DualListData.ListBegin(); auto NodeId = Node->Wrapped(ListBegin).ID(); - while (After != End) { - auto [RealNode, IROp] = After(); + while (Begin != End) { + auto [RealNode, IROp] = Begin(); uint8_t NumArgs = IR::GetArgs(IROp->Op); for (uint8_t i = 0; i < NumArgs; ++i) { @@ -47,7 +47,7 @@ void IREmitter::ReplaceAllUsesWithRange(OrderedNode *Node, OrderedNode *NewNode, } } - ++After; + ++Begin; } } diff --git a/External/FEXCore/include/FEXCore/IR/IREmitter.h b/External/FEXCore/include/FEXCore/IR/IREmitter.h index 6a9775be9..d0178caef 100644 --- a/External/FEXCore/include/FEXCore/IR/IREmitter.h +++ b/External/FEXCore/include/FEXCore/IR/IREmitter.h @@ -493,14 +493,15 @@ friend class FEXCore::IR::PassManager; // Because we are overwriting the node, we don't have to worry about update all the arguments which use it void ReplaceWithConstant(OrderedNode *Node, uint64_t Value); - void ReplaceAllUsesWithRange(OrderedNode *Node, OrderedNode *NewNode, AllNodesIterator After, AllNodesIterator End); + void ReplaceAllUsesWithRange(OrderedNode *Node, OrderedNode *NewNode, AllNodesIterator Begin, AllNodesIterator End); void ReplaceUsesWithAfter(OrderedNode *Node, OrderedNode *NewNode, AllNodesIterator After) { + ++After; ReplaceAllUsesWithRange(Node, NewNode, After, AllNodesIterator(DualListData.ListBegin(), DualListData.DataBegin())); } void ReplaceUsesWithAfter(OrderedNode *Node, OrderedNode *NewNode, OrderedNode *After) { - auto Wrapped = Node->Wrapped(DualListData.ListBegin()); + auto Wrapped = After->Wrapped(DualListData.ListBegin()); AllNodesIterator It = AllNodesIterator(DualListData.ListBegin(), DualListData.DataBegin(), Wrapped); ReplaceUsesWithAfter(Node, NewNode, It); @@ -509,7 +510,7 @@ friend class FEXCore::IR::PassManager; void ReplaceAllUsesWith(OrderedNode *Node, OrderedNode *NewNode) { auto Start = AllNodesIterator(DualListData.ListBegin(), DualListData.DataBegin(), Node->Wrapped(DualListData.ListBegin())); - ReplaceUsesWithAfter(Node, NewNode, Start); + ReplaceAllUsesWithRange(Node, NewNode, Start, AllNodesIterator(DualListData.ListBegin(), DualListData.DataBegin())); LOGMAN_THROW_A_FMT(Node->NumUses == 0, "Node still used"); From 93de38d13c6878026e5eada4c5b88a2fb7ebf35f Mon Sep 17 00:00:00 2001 From: Scott Mansell Date: Mon, 13 Sep 2021 15:26:52 +1200 Subject: [PATCH 5/6] FillRegister: Keep refrence to original SSA This allows the RA Validation pass to verify that a spill slot contains the correct SSA. I was originally planning to do a IR equlivent test between the IR before and after register allocation, mostly to catch this type of error. But this approach was much faster to implement and gives 90% of the benefits. --- External/FEXCore/Source/Interface/IR/IR.json | 9 ++++++++- .../Interface/IR/Passes/RAValidation.cpp | 19 ++++++++++++------- .../IR/Passes/RegisterAllocationPass.cpp | 8 ++++++-- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/External/FEXCore/Source/Interface/IR/IR.json b/External/FEXCore/Source/Interface/IR/IR.json index fc5b527a9..6fb065e60 100644 --- a/External/FEXCore/Source/Interface/IR/IR.json +++ b/External/FEXCore/Source/Interface/IR/IR.json @@ -605,10 +605,17 @@ "FillRegister": { "Desc": ["Fills a register from a spill slot", "Spill slots are register allocated and has live ranges calculated to handle slot calculation", - "```diff\n- !Don't use this op. It is for RA to handle spilling and filling!\n```" + "```diff\n- !Don't use this op. It is for RA to handle spilling and filling!\n```", + "", + "The OriginalValue SSA arg points at the original SSA value spilled, and only exists for", + "RA validation purposes" ], "OpClass": "Memory", + "SSAArgs": "1", + "SSANames": [ + "OriginalValue" + ], "HasDest": true, "DestClass": "Complex", "Args": [ diff --git a/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp b/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp index a669382b0..2b7690c08 100644 --- a/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp +++ b/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp @@ -309,19 +309,24 @@ bool RAValidation::Run(IREmitter *IREmit) { case OP_FILLREGISTER: { auto FillRegister = IROp->C(); - uint32_t value = BlockRegState.Unspill(FillRegister->Slot); + uint32_t ExpectedValue = FillRegister->OriginalValue.ID(); + uint32_t Value = BlockRegState.Unspill(FillRegister->Slot); // TODO: This only proves that the Spill has a consistent SSA value // In the future we need to prove it contains the correct SSA value - if (value == RegState::UninitializedValue) { + if (Value == RegState::UninitializedValue) { HadError |= true; - Errors << fmt::format("%ssa{}: FillRegister; Spill Slot {} was undefined in at least one control flow path\n", - ID, FillRegister->Slot); - } else if (value == RegState::ClobberedValue) { + Errors << fmt::format("%ssa{}: FillRegister expected %ssa{} in Slot {}, but was undefined in at least one control flow path\n", + ID, ExpectedValue, FillRegister->Slot); + } else if (Value == RegState::ClobberedValue) { HadError |= true; - Errors << fmt::format("%ssa{}: FillRegister; Spill Slot {} contents vary depending on control flow\n", - ID, FillRegister->Slot); + Errors << fmt::format("%ssa{}: FillRegister expected %ssa{} in Slot {}, but contents vary depending on control flow\n", + ID, ExpectedValue, FillRegister->Slot); + } else if (Value != ExpectedValue) { + HadError |= true; + Errors << fmt::format("%ssa{}: FillRegister expected %ssa{} in Slot {}, but it actually contains %ssa{}\n", + ID, ExpectedValue, FillRegister->Slot, Value); } break; } diff --git a/External/FEXCore/Source/Interface/IR/Passes/RegisterAllocationPass.cpp b/External/FEXCore/Source/Interface/IR/Passes/RegisterAllocationPass.cpp index d6f08df3e..20791b22c 100644 --- a/External/FEXCore/Source/Interface/IR/Passes/RegisterAllocationPass.cpp +++ b/External/FEXCore/Source/Interface/IR/Passes/RegisterAllocationPass.cpp @@ -427,6 +427,10 @@ namespace FEXCore::IR { // Set this node's block ID Graph->Nodes[Node].Head.BlockID = BlockNodeID; + // FillRegister's SSA arg is only there for verification, and we don't want it + // to impact the live range. + if (IROp->Op == OP_FILLREGISTER) continue; + uint8_t NumArgs = IR::GetArgs(IROp->Op); for (uint8_t i = 0; i < NumArgs; ++i) { if (IROp->Args[i].IsInvalid()) continue; @@ -1336,10 +1340,10 @@ namespace FEXCore::IR { IREmit->SetWriteCursor(FirstUseOrderedNode); - auto FilledInterference = IREmit->_FillRegister(SpillSlot, InterferenceRegClass); + auto FilledInterference = IREmit->_FillRegister(InterferenceOrderedNode, SpillSlot, InterferenceRegClass); FilledInterference.first->Header.Size = InterferenceIROp->Size; FilledInterference.first->Header.ElementSize = InterferenceIROp->ElementSize; - IREmit->ReplaceUsesWithAfter(InterferenceOrderedNode, FilledInterference, FirstUseLocation); + IREmit->ReplaceUsesWithAfter(InterferenceOrderedNode, FilledInterference, FilledInterference); Spilled = true; } } From 486c62f77cc05d3833a9df88900d1f4768001766 Mon Sep 17 00:00:00 2001 From: Scott Mansell Date: Mon, 13 Sep 2021 20:25:33 +1200 Subject: [PATCH 6/6] RAValidation: Improve comments --- .../Interface/IR/Passes/RAValidation.cpp | 40 ++++++++++++++----- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp b/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp index 2b7690c08..501c13123 100644 --- a/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp +++ b/External/FEXCore/Source/Interface/IR/Passes/RAValidation.cpp @@ -14,6 +14,7 @@ namespace FEXCore::IR::Validation { +// Hold the mapping of physical registers to the SSA id it holds at any given point in the IR struct RegState { static constexpr uint32_t UninitializedValue = 0; static constexpr uint32_t InvalidReg = 0xffff'ffff; @@ -29,9 +30,12 @@ struct RegState { // These assumptions were all true for the state of the arm64 and x86 jits at the time this was written + // Mark a physical register as containing a SSA id bool Set(PhysicalRegister Reg, uint32_t ssa) { LOGMAN_THROW_A(ssa != 0, "RegState assumes ssa0 will be the block header and never assigned to a register"); + // PhyscialRegisters aren't fully mapped until assembly emission + // We need to apply a generic mapping here to catch any aliasing switch (Reg.Class) { case GPRClass: GPRs[Reg.Reg] = ssa; @@ -59,6 +63,8 @@ struct RegState { return false; } + // Get the current SSA id + // Or an error value there isn't a (sane) SSA id uint32_t Get(PhysicalRegister Reg) { switch (Reg.Class) { case GPRClass: @@ -89,10 +95,12 @@ struct RegState { } + // Mark a spill slot as containing a SSA id void Spill(uint32_t SpillSlot, uint32_t ssa) { Spills[SpillSlot] = ssa; } + // Consume (and return) the SSA id currently in a spill slot uint32_t Unspill(uint32_t SpillSlot) { if (Spills.contains(SpillSlot)) { uint32_t Value = Spills[SpillSlot]; @@ -102,6 +110,13 @@ struct RegState { return UninitializedValue; } + // Intersect another regstate with this one + // Any registers/slots which contain the same SSA id will be persevered + // Anything else will be marked as Clobbered + // + // Useful for merging two branches of control flow. + // Any register that differs depending on control flow shouldn't be consumed by + // code that follows void Intersect(RegState& other) { for (size_t i = 0; i < GPRs.size(); i++) { if (GPRs[i] != other.GPRs[i]) { @@ -125,6 +140,9 @@ struct RegState { } } + // Filter out all registers/slots containing an SSA id larger than MaxSSA + // Mark them as Clobbered. + // Useful for backwards edges, where using an SSA from before the void Filter(uint32_t MaxSSA) { for (auto &gpr : GPRs) { if (gpr > MaxSSA) { @@ -153,7 +171,7 @@ private: std::unordered_map Spills; public: - uint32_t Version{}; + uint32_t Version{}; // Used to force regeneration of RegStates after following backward edges }; class RAValidation final : public FEXCore::IR::Pass { @@ -162,7 +180,10 @@ public: bool Run(IREmitter *IREmit) override; private: + // Holds the calculated RegState at the exit of each block std::unordered_map BlockExitState; + + // A queue of blocks we need to visit (or revisit) std::deque BlocksToVisit; }; @@ -172,6 +193,7 @@ bool RAValidation::Run(IREmitter *IREmit) { IR::RegisterAllocationData* RAData = Manager->GetPass("RA")->GetAllocationData(); BlockExitState.clear(); + // BlocksToVisit will already be empty // Get the control flow graph from the validation pass auto ValidationPass = Manager->GetPass("IRValidation"); @@ -195,14 +217,12 @@ bool RAValidation::Run(IREmitter *IREmit) { auto& BlockInfo = OffsetToBlockMap[BlockID]; auto IsFowardsEdge = [&] (uint32_t PredecessorID) { - // TODO: This isn't the best definition of fowards/backwards edges. It's possible for - // Blocks to be out of order. Will need a proper CFG analysis pass. - // - // But I don't think we currently generate Backwards branches that don't follow this rule + // Blocks are sorted in FEXes IR, so backwards edges always go to a lower (or equal) Block ID return PredecessorID < BlockID; }; - // First, make sure we have the exit state data for all Predecessor + // First, make sure we have the exit state for all Predecessors that + // get here via a forwards branch. bool MissingPredecessor = false; for (auto Predecessor : BlockInfo.Predecessors) { @@ -224,7 +244,8 @@ bool RAValidation::Run(IREmitter *IREmit) { continue; } - // Remove block from queue + // We have committed to processing this block + // Remove from queue BlocksToVisit.pop_front(); bool FirstVisit = !BlockExitState.contains(BlockID); @@ -349,10 +370,7 @@ bool RAValidation::Run(IREmitter *IREmit) { for (auto Successor : BlockInfo.Successors) { auto SuccessorID = CurrentIR.GetID(Successor); - // TODO: This isn't the best definition of fowards/backwards edges. It's possible for - // Blocks to be out of order. Will need a proper CFG analysis pass. - // - // But I don't think we currently generate Backwards branches that don't follow this rule + // Blocks are sorted in FEXes IR, so backwards edges always go to a lower (or equal) Block ID bool FowardsEdge = SuccessorID > BlockID; if (FowardsEdge) {