feat(evm): implement true-SSA stack lifting for EVM multipass JIT#395
Open
ZR74 wants to merge 14 commits intoDTVMStack:mainfrom
Open
feat(evm): implement true-SSA stack lifting for EVM multipass JIT#395ZR74 wants to merge 14 commits intoDTVMStack:mainfrom
ZR74 wants to merge 14 commits intoDTVMStack:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements a conservative Phase-1 EVM stack-value lifting pass in the multipass JIT frontend. It extends the EVMAnalyzer with control-flow graph (CFG) edges, per-block stack depth propagation, JUMPDEST canonicalization, and liftability flags, then wires those results into EVMByteCodeVisitor and EVMMirBuilder so stack values can flow across safe constant-jump and fallthrough edges without immediate runtime stack materialization. The feature is opt-in behind a new CMake flag (ZEN_ENABLE_EVM_STACK_SSA_LIFT) and a focused regression test suite is added.
Changes:
- New
ZEN_ENABLE_EVM_STACK_SSA_LIFTCMake option and compile-time definition added to the root andsrcbuild files. EVMAnalyzerheavily refactored into multiple passes (suitability, JUMPDEST-run canonicalization, block building, predecessor linking, depth propagation, liftability finalization) andBlockInfoextended with CFG and lifted-stack metadata.EVMByteCodeVisitorextended with lifted-block tracking, logical-to-runtime stack materialization, and per-edge lifted entry-state assignment;EVMMirBuildergets four new stack helpers; newevmJitFrontendTeststarget added to the test build.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Adds ZEN_ENABLE_EVM_STACK_SSA_LIFT option |
| src/CMakeLists.txt | Adds add_definitions for the new flag |
| src/tests/CMakeLists.txt | Adds evmJitFrontendTests build/link/test rules guarded by ZEN_ENABLE_EVM + ZEN_ENABLE_MULTIPASS_JIT |
| src/compiler/evm_frontend/evm_analyzer.h | Full refactor: CFG building, depth propagation, liftability computation |
| src/compiler/evm_frontend/evm_mir_compiler.h | Declares four new builder helpers for lifted stack support |
| src/compiler/evm_frontend/evm_mir_compiler.cpp | Implements setTrackedStackDepth, createStackEntryOperand, assignStackEntryOperand, spillTrackedStack |
| src/action/evm_bytecode_visitor.h | Lifted-block visitor logic, drainLogicalStack/restoreLogicalStack/finalizeBlockExit, per-opcode lifted-edge assignments |
| src/tests/evm_jit_frontend_tests.cpp | New test suite with MockEVMBuilder and analyzer/visitor regression tests |
| openspec/changes/add-evm-stack-ssa-lift/* | Proposal, design, spec, and task documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+583
to
+602
| while (true) { | ||
| BlockInfo Info(CurEntryPC, BodyStartPC, IsJumpDestBlock); | ||
| size_t ScanPC = BodyStartPC; | ||
| uint64_t NextEntryPC = 0; | ||
| size_t NextBodyStartPC = BytecodeSize; | ||
| bool HasNextBlock = false; | ||
| analyzeBlockBody(Info, Bytecode, BytecodeSize, ScanPC, NextEntryPC, | ||
| NextBodyStartPC, HasNextBlock); | ||
| BlockInfos[CurEntryPC] = Info; | ||
| if (!HasNextBlock) { | ||
| break; | ||
| } | ||
| CurEntryPC = NextEntryPC; | ||
| BodyStartPC = NextBodyStartPC; | ||
| IsJumpDestBlock = hasCanonicalJumpDest(CurEntryPC) && | ||
| getCanonicalJumpDestPC(CurEntryPC) == CurEntryPC; | ||
| if (BodyStartPC > BytecodeSize) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
In buildBlocks, when the very first opcode of the entry block (with EntryPC = 0, BodyStartPC = 0) is a JUMPI, the fallthrough block gets NextEntryPC = 0 (the JUMPI's PC). The loop then sets CurEntryPC = 0 and creates a new BlockInfo(0, 1, false) which overwrites the original entry block in BlockInfos[0]. This loses the JUMJI block's Successors, HasConditionalJump, and other fields. As a result, resolveEntryDepths will find only the overwritten block at EntryPC=0, never propagate stack depth to the jump target, and the BFS will produce incorrect liftability results for this control-flow pattern. A guard should ensure that a block is not created with an EntryPC equal to one that already exists (or the JUMPI fallthrough block's PC should be handled as CurPC + 1 rather than CurPC).
Comment on lines
262
to
+322
| @@ -169,157 +275,404 @@ class EVMAnalyzer { | |||
| evmc_get_instruction_names_table(zen::evm::DEFAULT_REVISION); | |||
| } | |||
|
|
|||
| // Initialize block info for the first block | |||
| BlockInfo CurInfo(0); | |||
|
|
|||
| // JIT suitability tracking state | |||
| size_t CurConsecutiveExpensive = 0; | |||
| size_t CurBlockExpensiveCount = 0; | |||
| bool PrevWasDup = false; | |||
|
|
|||
| while (Ip < IpEnd) { | |||
| evmc_opcode Opcode = static_cast<evmc_opcode>(*Ip); | |||
| size_t PCIndex = 0; | |||
| while (PCIndex < BytecodeSize) { | |||
| evmc_opcode Opcode = static_cast<evmc_opcode>(Bytecode[PCIndex]); | |||
| uint8_t OpcodeU8 = static_cast<uint8_t>(Opcode); | |||
| ptrdiff_t Diff = Ip - Bytecode; | |||
| PC = static_cast<uint64_t>(Diff >= 0 ? Diff : 0); | |||
|
|
|||
| Ip++; | |||
|
|
|||
| // --- JIT suitability: accumulate MIR estimate --- | |||
| JITResult.MirEstimate += MIR_OPCODE_WEIGHT[OpcodeU8]; | |||
|
|
|||
| // --- JIT suitability: RA-expensive pattern tracking --- | |||
| if (isRAExpensiveOpcode(OpcodeU8)) { | |||
| JITResult.RAExpensiveCount++; | |||
| CurInfo.RAExpensiveCount++; | |||
| CurBlockExpensiveCount++; | |||
| CurConsecutiveExpensive++; | |||
| // DUP feedback: previous opcode was DUP, now RA-expensive | |||
| if (PrevWasDup) { | |||
| JITResult.DupFeedbackPatternCount++; | |||
| } | |||
| PrevWasDup = false; | |||
| } else if (isDupOrSwapOpcode(OpcodeU8)) { | |||
| // DUP/SWAP are transparent — don't break consecutive run | |||
| PrevWasDup = isDupOpcode(OpcodeU8); | |||
| } else { | |||
| // Any other opcode breaks the consecutive run | |||
| JITResult.MaxConsecutiveExpensive = std::max( | |||
| JITResult.MaxConsecutiveExpensive, CurConsecutiveExpensive); | |||
| CurConsecutiveExpensive = 0; | |||
| PrevWasDup = false; | |||
| } | |||
|
|
|||
| // Check if opcode is undefined for current revision | |||
| bool IsBlockBoundary = (Opcode == OP_JUMPI || Opcode == OP_JUMPDEST || | |||
| isBlockTerminator(Opcode)); | |||
| if (IsBlockBoundary) { | |||
| JITResult.MaxBlockExpensiveCount = | |||
| std::max(JITResult.MaxBlockExpensiveCount, CurBlockExpensiveCount); | |||
| CurBlockExpensiveCount = 0; | |||
| JITResult.MaxConsecutiveExpensive = std::max( | |||
| JITResult.MaxConsecutiveExpensive, CurConsecutiveExpensive); | |||
| CurConsecutiveExpensive = 0; | |||
| PrevWasDup = false; | |||
| } | |||
|
|
|||
| size_t PushBytes = immediateSize(Opcode); | |||
| PCIndex += 1 + PushBytes; | |||
|
|
|||
| (void)InstructionMetrics; | |||
| (void)InstructionNames; | |||
There was a problem hiding this comment.
In analyzeSuitability, InstructionMetrics and InstructionNames are retrieved (with fallback logic) but are never actually used in the function body — they are only suppressed with (void) casts at lines 321-322. The original code used them for undefined instruction tracking and stack metric computation, but in the refactored version these responsibilities moved to analyzeBlockBody. The dead fetch and the fallback assignments to InstructionNames/InstructionMetrics (lines 266-276) should be removed.
Comment on lines
+360
to
+368
| void ensureAbstractDepth(std::vector<AbstractValue> &Stack, | ||
| size_t &EntryDepth, size_t RequiredDepth) { | ||
| if (Stack.size() >= RequiredDepth) { | ||
| return; | ||
| } | ||
| size_t Deficit = RequiredDepth - Stack.size(); | ||
| Stack.insert(Stack.begin(), Deficit, AbstractValue::unknown()); | ||
| EntryDepth += Deficit; | ||
| } |
There was a problem hiding this comment.
The ensureAbstractDepth function uses Stack.insert(Stack.begin(), ...) to prepend elements to a std::vector. This is O(n) per call because it shifts all existing elements. While the EVM stack is bounded at 1024 and this runs at compile time, the pattern is called repeatedly within analyzeBlockBody for each opcode that requires more stack depth than currently tracked, which could cause O(n²) behavior in degenerate cases where the block gradually reveals a deep pre-existing stack. Consider using a deque or prepending to a separate "prefix" vector to avoid repeated O(n) shifts.
src/action/evm_bytecode_visitor.h
Outdated
| bool InDeadCode = false; | ||
| uint64_t PC = 0; | ||
| bool CurrentBlockLifted = false; | ||
| std::map<uint64_t, bool> LiftedBlocks; |
There was a problem hiding this comment.
The LiftedBlocks member is declared as std::map<uint64_t, bool> but is used purely as a set (values are always true when inserted, and isLiftedBlock checks It->second). Using a std::set<uint64_t> or std::unordered_set<uint64_t> would be more semantically clear and slightly more efficient by removing the unused boolean storage. The rest of the codebase uses std::map for block data that has associated values (e.g. LiftedEntryStates), so this inconsistency stands out.
Comment on lines
+852
to
+862
| bool tryAssignFallthroughEntryState(const EVMAnalyzer &Analyzer, | ||
| uint64_t SuccPC) { | ||
| (void)Analyzer; | ||
| if (!CurrentBlockLifted || !isLiftedBlock(SuccPC)) { | ||
| return false; | ||
| } | ||
| auto OutgoingStack = drainLogicalStack(); | ||
| assignLiftedEntryState(SuccPC, OutgoingStack); | ||
| finalizeBlockExit(std::move(OutgoingStack), false); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The tryAssignFallthroughEntryState method declares a const EVMAnalyzer &Analyzer parameter but immediately suppresses it with (void)Analyzer without using it anywhere in the function body. This unused parameter should be removed from the function signature and all call sites updated accordingly.
⚡ Performance Regression Check Results✅ Performance Check Passed (interpreter)Performance Benchmark Results (threshold: 25%)
Summary: 194 benchmarks, 0 regressions ✅ Performance Check Passed (multipass)Performance Benchmark Results (threshold: 25%)
Summary: 194 benchmarks, 0 regressions |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+658
to
+662
| if (SuccInfo.ResolvedEntryStackDepth < 0) { | ||
| SuccInfo.ResolvedEntryStackDepth = ExitDepth; | ||
| WorkList.push(Succ); | ||
| } else if (SuccInfo.ResolvedEntryStackDepth != ExitDepth) { | ||
| SuccInfo.HasInconsistentEntryDepth = true; |
Comment on lines
275
to
277
| const auto *InstructionMetrics = | ||
| evmc_get_instruction_metrics_table(Revision); | ||
| const auto *InstructionNames = evmc_get_instruction_names_table(Revision); |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add compatibility shims for legacy frontend test builders so the SSA frontend changes still compile in evmJitFrontendTests.\n\nStop undefined-opcode analysis at the first undefined instruction and let the visitor trap at the real opcode position instead of rejecting the whole block at entry. This avoids bogus stack-overflow traps in multipass external_vm tests and keeps the frontend regressions covered with updated analyzer expectations and a new undefined-opcode test.
Replace the heavy compiler common header in the EVM analyzer with the lightweight common defines header. This keeps runtime-side users such as evm_module building in CI jobs that do not expose LLVM headers on the runtime compile path while preserving the analyzer behavior.
ZR74
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):
2. What is the scope of this PR (e.g. component or file name):
3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):
4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):
5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:
6. Release note