From 4448f84f29b7a7fb9aab2e308a8053b23e2649c6 Mon Sep 17 00:00:00 2001 From: Alyssa Rosenzweig Date: Tue, 21 May 2024 18:47:51 -0400 Subject: [PATCH] IRValidation: merge in ValueDominanceValidation All we actually need to validate is that each source has been previously defined within the block. That checks everything we care about now. Signed-off-by: Alyssa Rosenzweig --- FEXCore/Source/CMakeLists.txt | 1 - FEXCore/Source/Interface/IR/PassManager.cpp | 1 - FEXCore/Source/Interface/IR/Passes.h | 1 - .../Interface/IR/Passes/IRValidation.cpp | 34 +++--- .../IR/Passes/ValueDominanceValidation.cpp | 106 ------------------ docs/SourceOutline.md | 1 - 6 files changed, 18 insertions(+), 126 deletions(-) delete mode 100644 FEXCore/Source/Interface/IR/Passes/ValueDominanceValidation.cpp diff --git a/FEXCore/Source/CMakeLists.txt b/FEXCore/Source/CMakeLists.txt index ce4b17b25..425a56b8d 100644 --- a/FEXCore/Source/CMakeLists.txt +++ b/FEXCore/Source/CMakeLists.txt @@ -144,7 +144,6 @@ set (SRCS Interface/IR/Passes/IRValidation.cpp Interface/IR/Passes/RAValidation.cpp Interface/IR/Passes/LongDivideRemovalPass.cpp - Interface/IR/Passes/ValueDominanceValidation.cpp Interface/IR/Passes/RedundantFlagCalculationElimination.cpp Interface/IR/Passes/DeadStoreElimination.cpp Interface/IR/Passes/RegisterAllocationPass.cpp diff --git a/FEXCore/Source/Interface/IR/PassManager.cpp b/FEXCore/Source/Interface/IR/PassManager.cpp index efe9a6d28..249898e49 100644 --- a/FEXCore/Source/Interface/IR/PassManager.cpp +++ b/FEXCore/Source/Interface/IR/PassManager.cpp @@ -97,7 +97,6 @@ void PassManager::AddDefaultValidationPasses() { #if defined(ASSERTIONS_ENABLED) && ASSERTIONS_ENABLED InsertValidationPass(Validation::CreateIRValidation(), "IRValidation"); InsertValidationPass(Validation::CreateRAValidation()); - InsertValidationPass(Validation::CreateValueDominanceValidation()); #endif } diff --git a/FEXCore/Source/Interface/IR/Passes.h b/FEXCore/Source/Interface/IR/Passes.h index c47459a80..949ebb557 100644 --- a/FEXCore/Source/Interface/IR/Passes.h +++ b/FEXCore/Source/Interface/IR/Passes.h @@ -29,7 +29,6 @@ fextl::unique_ptr CreateLongDivideEliminationPass(); namespace Validation { fextl::unique_ptr CreateIRValidation(); fextl::unique_ptr CreateRAValidation(); - fextl::unique_ptr CreateValueDominanceValidation(); } // namespace Validation namespace Debug { diff --git a/FEXCore/Source/Interface/IR/Passes/IRValidation.cpp b/FEXCore/Source/Interface/IR/Passes/IRValidation.cpp index 7c2e0c1b1..76d6c8897 100644 --- a/FEXCore/Source/Interface/IR/Passes/IRValidation.cpp +++ b/FEXCore/Source/Interface/IR/Passes/IRValidation.cpp @@ -46,11 +46,12 @@ bool IRValidation::Run(IREmitter* IREmit) { OffsetToBlockMap.clear(); EntryBlock = nullptr; - if (CurrentIR.GetSSACount() > MaxNodes) { - NodeIsLive.Realloc(CurrentIR.GetSSACount()); + uint32_t Count = CurrentIR.GetSSACount(); + if (Count > MaxNodes) { + NodeIsLive.Realloc(Count); } - fextl::vector Uses(CurrentIR.GetSSACount(), 0); + fextl::vector Uses(Count, 0); #if defined(ASSERTIONS_ENABLED) && ASSERTIONS_ENABLED auto HeaderOp = CurrentIR.GetHeader(); @@ -62,8 +63,6 @@ bool IRValidation::Run(IREmitter* IREmit) { RAData = Manager->GetPass("RA")->GetAllocationData(); } - NodeIsLive.Set(1); // IRHEADER - for (auto [BlockNode, BlockHeader] : CurrentIR.GetBlocks()) { auto BlockIROp = BlockHeader->CW(); LOGMAN_THROW_AA_FMT(BlockIROp->Header.Op == OP_CODEBLOCK, "IR type failed to be a code block"); @@ -75,6 +74,9 @@ bool IRValidation::Run(IREmitter* IREmit) { const auto BlockID = CurrentIR.GetID(BlockNode); BlockInfo* CurrentBlock = &OffsetToBlockMap.try_emplace(BlockID).first->second; + // We only allow defs local to a single block, so clear live set per block + NodeIsLive.MemClear(Count); + for (auto [CodeNode, IROp] : CurrentIR.GetCode(BlockNode)) { const auto ID = CurrentIR.GetID(CodeNode); const uint8_t OpSize = IROp->Size; @@ -125,21 +127,21 @@ bool IRValidation::Run(IREmitter* IREmit) { for (uint32_t i = 0; i < NumArgs; ++i) { OrderedNodeWrapper Arg = IROp->Args[i]; const auto ArgID = Arg.ID(); - - // Was an argument defined after this node? - if (ArgID >= ID) { - HadError |= true; - Errors << "%" << ID << ": Arg[" << i << "] has definition after use at %" << ArgID << std::endl; - } - - if (ArgID.IsValid() && !NodeIsLive.Get(ArgID.Value)) { - HadError |= true; - Errors << "%" << ID << ": Arg[" << i << "] references dead %" << ArgID << std::endl; - } + IROps Op = CurrentIR.GetOp(Arg)->Op; if (ArgID.IsValid()) { Uses[ArgID.Value]++; } + + // We do not validate the location of inline constants because it's + // irrelevant, they're ignored by RA and always inlined to where they + // need to be. This lets us pool inline constants globally. + bool Ignore = (Op == OP_IRHEADER || Op == OP_INLINECONSTANT); + + if (!Ignore && ArgID.IsValid() && !NodeIsLive.Get(ArgID.Value)) { + HadError |= true; + Errors << "%" << ID << ": Arg[" << i << "] references invalid %" << ArgID << std::endl; + } } NodeIsLive.Set(ID.Value); diff --git a/FEXCore/Source/Interface/IR/Passes/ValueDominanceValidation.cpp b/FEXCore/Source/Interface/IR/Passes/ValueDominanceValidation.cpp deleted file mode 100644 index dd8939ff4..000000000 --- a/FEXCore/Source/Interface/IR/Passes/ValueDominanceValidation.cpp +++ /dev/null @@ -1,106 +0,0 @@ -// SPDX-License-Identifier: MIT -/* -$info$ -tags: ir|opts -desc: Sanity Checking -$end_info$ -*/ - -#include "Interface/IR/IR.h" -#include "Interface/IR/IREmitter.h" -#include "Interface/IR/PassManager.h" - -#include -#include -#include -#include -#include -#include -#include - -#include -#include -#include -#include - -namespace FEXCore::IR::Validation { -class ValueDominanceValidation final : public FEXCore::IR::Pass { -public: - bool Run(IREmitter* IREmit) override; -}; - -bool ValueDominanceValidation::Run(IREmitter* IREmit) { - FEXCORE_PROFILE_SCOPED("PassManager::ValueDominanceValidation"); - - bool HadError = false; - auto CurrentIR = IREmit->ViewIR(); - - fextl::ostringstream Errors; - - for (auto [BlockNode, BlockHeader] : CurrentIR.GetBlocks()) { - auto BlockIROp = BlockHeader->CW(); - - for (auto [CodeNode, IROp] : CurrentIR.GetCode(BlockNode)) { - const auto CodeID = CurrentIR.GetID(CodeNode); - - const uint8_t NumArgs = IR::GetRAArgs(IROp->Op); - for (uint32_t i = 0; i < NumArgs; ++i) { - if (IROp->Args[i].IsInvalid()) { - continue; - } - - // We do not validate the location of inline constants because it's - // irrelevant, they're ignored by RA and always inlined to where they - // need to be. This lets us pool inline constants globally. - IROps Op = CurrentIR.GetOp(IROp->Args[i])->Op; - if (Op == OP_IRHEADER || Op == OP_INLINECONSTANT) { - continue; - } - - OrderedNodeWrapper Arg = IROp->Args[i]; - - // If the SSA argument is not defined INSIDE the block, we have - // cross-block liveness, which we forbid in the IR to simplify RA. - if (!(Arg.ID() >= BlockIROp->Begin.ID() && Arg.ID() < BlockIROp->Last.ID())) { - HadError |= true; - Errors << "Inst %" << CodeID << ": Arg[" << i << "] %" << Arg.ID() << " definition not local!" << std::endl; - continue; - } - - // The SSA argument is defined INSIDE this block. - // It must only be declared prior to this instruction - // Eg: Valid - // CodeBlock_1: - // %_1 = Load - // %_2 = Load - // %_3 = %_1, %_2 - // - // Eg: Invalid - // CodeBlock_1: - // %_1 = Load - // %_2 = %_1, %_3 - // %_3 = Load - if (Arg.ID() > CodeID) { - HadError |= true; - Errors << "Inst %" << CodeID << ": Arg[" << i << "] %" << Arg.ID() << " definition does not dominate this use!" << std::endl; - } - } - } - } - - if (HadError) { - fextl::stringstream Out; - FEXCore::IR::Dump(&Out, &CurrentIR, nullptr); - Out << "Errors:" << std::endl << Errors.str() << std::endl; - LogMan::Msg::EFmt("{}", Out.str()); - LOGMAN_MSG_A_FMT("Encountered IR validation Error"); - } - - return false; -} - -fextl::unique_ptr CreateValueDominanceValidation() { - return fextl::make_unique(); -} - -} // namespace FEXCore::IR::Validation diff --git a/docs/SourceOutline.md b/docs/SourceOutline.md index 4d5a15b81..82df33d83 100644 --- a/docs/SourceOutline.md +++ b/docs/SourceOutline.md @@ -119,7 +119,6 @@ IR to IR Optimization - [RedundantFlagCalculationElimination.cpp](../FEXCore/Source/Interface/IR/Passes/RedundantFlagCalculationElimination.cpp): This is not used right now, possibly broken - [RegisterAllocationPass.cpp](../FEXCore/Source/Interface/IR/Passes/RegisterAllocationPass.cpp) - [RegisterAllocationPass.h](../FEXCore/Source/Interface/IR/Passes/RegisterAllocationPass.h) -- [ValueDominanceValidation.cpp](../FEXCore/Source/Interface/IR/Passes/ValueDominanceValidation.cpp): Sanity Checking ### opcodes