feat(mlir): enable the spill-retry protocol for the MLIR pipeline by abinavpp · Pull Request #653 · NomicFoundation/solx

abinavpp · 2026-08-16T13:10:19Z

Copilot

Pull request overview

This PR updates the MLIR-based compilation pipeline to support the spill-retry protocol by keeping memoryguard symbolic through MLIR lowering and then folding it into a constant in the LLVM module before LLVM optimization runs.

Changes:

Updates the MLIR convert-yul-to-std pass binding and usage to keep memoryguard symbolic via a new symbolic_mem_guard parameter.
Adds a pre-optimization fold step in the EVM codegen context to rewrite llvm.evm.memoryguard calls into constants based on spill_area_size.
Updates Sol dialect array type construction stub to the newer API shape (optional fixed-size vs dynamic).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
solx-mlir/src/ffi.rs	Updates the MLIR pass FFI binding to accept `symbolic_mem_guard`.
solx-mlir/src/context/mod.rs	Uses the new pass constructor and documents symbolic memoryguard behavior in the pass pipeline.
solx-mlir/sol_attr_stubs.cpp	Updates Sol dialect `ArrayType` construction to use an optional size representation.
solx-core/src/project/contract/mod.rs	Folds symbolic memoryguard in the MLIR→LLVM path before running optimization/codegen.
solx-codegen-evm/src/codegen/context/mod.rs	Introduces `fold_memory_guard` to rewrite `llvm.evm.memoryguard` into constants and capture the guard value.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

github-actions · 2026-08-16T13:22:58Z

Coverage Summary

Crate	Line Coverage	Function Coverage
solx	🟢 83.6%	🔴 20.0%
solx-benchmark-converter	🔴 0.0%	🔴 0.0%
solx-codegen-evm	🔴 26.1%	🔴 15.0%
solx-compiler-downloader	🔴 0.0%	🔴 0.0%
solx-core	🔴 38.6%	🔴 46.9%
solx-dev	🔴 2.4%	🔴 3.0%
solx-evm-assembly	🔴 0.0%	🔴 0.0%
solx-mlir	🟡 59.2%	🟡 55.1%
solx-slang	🔴 23.4%	🔴 34.1%
solx-solc-test-adapter	🔴 1.7%	🔴 2.1%
solx-standard-json	🔴 42.8%	🔴 47.7%
solx-tester	🔴 36.2%	🔴 34.0%
solx-utils	🔴 29.0%	🔴 32.5%
solx-yul	🔴 0.0%	🔴 0.0%
Total	🔴 10.9%	🔴 13.2%

Codecov Report | HTML Report | Workflow Run

hedgar2017

Thanks! Looks good, but could you run cargo run-tester-slang and post the test tally? AFAIU, it should have unblocked some stack-too-deep tests?

abinavpp · 2026-08-20T08:54:09Z

cargo run-tester-slang: 10243 -> 10247 passed (245 -> 241 failed, 474 invalid unchanged, 10962 total).

The delta is one file, tests/solidity/simple/algorithm/arrays/standard_functions_high_order.sol (4 cases). It is exactly the silent case this PR is about: the retry used to re-run codegen with the spill region pinned at 0x80, i.e. under the heap, so the filter/complex calls returned corrupted arrays instead of aborting. Nothing else changes state, and the corpus surfaces no other spilling contract at M3B3.

Measured with the two commits cherry-picked onto main-slang (6387d7e), since the branch as pushed is still main-based, solx-solidity at the pinned submodule commit, and the paired MLIR install from solx-llvm#142.

abinavpp · 2026-08-20T12:34:16Z

Correction to the tally above, after rebasing the MLIR side onto current main.

Baseline and branch are now identical: 10247 passed / 241 failed / 474 invalid, same failing set - so this PR's tester delta is 0.

The +4 I reported was not this PR. It came from solx-llvm main's [mlir][Sol] Build precise per-type dispatch tables for internal fnptr calls, which my install predated: with that commit, standard_functions_high_order.sol stops spilling altogether and passes on plain main-slang. Measured both ways - it flips on the baseline binary as soon as the install carries that commit.

So the corpus currently contains no test whose outcome depends on the retry path. What the fix buys is still real, just unobservable here: under the pre-dispatch-table MLIR that file was the demonstration (corrupted arrays -> pass), and the hand-written spill inputs behave correctly (reload reuse and the return buffer land at G+S, a non-spilling contract keeps FMP at 0x80). Cost is nil: a sequential sweep of tests/solidity plus the upstream semanticTests (2891 files) shows no systematic compile-time difference.

hedgar2017

LGTM, but please get merged LLVM first.

vladimirradosavljevic · 2026-09-02T08:32:17Z

+        use inkwell::values::AsValueRef;
+
+        let size = context.i64_type().const_int(size, false);
+        for flag in module.get_global_metadata("llvm.module.flags") {


Can we use module.get_flag instead?

The MLIR pipeline keeps the memoryguard symbolic through the conversion, and the EVM backend's evm-fold-memory-guard pass folds it and publishes the spill region's base, so the retry no longer places the region under the heap.

The pipeline needs evm-fold-memory-guard, which the pinned revision predates.

Both front ends now name the region on the module rather than on the command line, so the backend has one interface and the legacy pipeline stops depending on process-global options. The MLIR front end publishes the guard itself, since only it knows where its static allocations end.

abinavpp · 2026-09-04T13:39:58Z

The main-solc copy of this is up as #690.

hedgar2017 · 2026-09-05T21:41:56Z

+    ///
+    /// Whether the `llvm.module.flags` triplet `flag` is named `key`.
+    ///
+    fn is_flag_named(flag: &inkwell::values::MetadataValue<'ctx>, key: &str) -> bool {


Single call: please inline.

hedgar2017 · 2026-09-05T21:43:59Z

+    /// Whether the `llvm.module.flags` triplet `flag` is named `key`.
+    ///
+    fn is_flag_named(flag: &inkwell::values::MetadataValue<'ctx>, key: &str) -> bool {
+        flag.get_node_values().get(1).is_some_and(|name| {


Worth explaining why is this index == 1, using a named constant, and/or using a stricter / more typed accessor.

hedgar2017 · 2026-09-05T21:46:05Z

+    /// Sets the spill region module flags read by the EVM backend.
+    ///
+    fn describe_stack_region(&self, size: u64) {
+        if self.module().get_flag("evm-memory-guard").is_none() {


Is this guard needed, or is the setter idempotent?
If it's needed, worth spelling the magic literal once, or even refactoring these keys by moving them to some enum as their number grows.

hedgar2017 · 2026-09-05T21:50:38Z

+    ///
+    /// Adds the `key` module flag, or replaces its value if it is already there.
+    ///
+    fn set_module_flag(&self, key: &str, value: u64) {


This is more low-level so belongs in inkwell. Maybe it already contains some flag setters in the module.

hedgar2017 · 2026-09-05T21:52:38Z

+    ///
+    /// Sets the spill region module flags read by the EVM backend.
+    ///
+    fn describe_stack_region(&self, size: u64) {


I think can also be inlined once set_module_flag is moved to inkwell.

abinavpp requested a lite review from Copilot August 16, 2026 13:10

Copilot started reviewing on behalf of abinavpp August 16, 2026 13:11 View session

Copilot AI reviewed Aug 16, 2026

View reviewed changes

Comment thread solx-mlir/sol_attr_stubs.cpp Outdated

Comment thread solx-mlir/src/context/mod.rs Outdated

abinavpp force-pushed the app-mlir-spill branch from 2f831d3 to e43f9a2 Compare August 18, 2026 21:11

abinavpp marked this pull request as ready for review August 19, 2026 07:04

abinavpp requested review from PavelKopyl and vladimirradosavljevic August 19, 2026 07:04

hedgar2017 changed the base branch from main to main-slang August 19, 2026 12:57

hedgar2017 requested changes Aug 19, 2026

View reviewed changes

hedgar2017 force-pushed the app-mlir-spill branch 2 times, most recently from 14877f9 to 935e266 Compare August 20, 2026 12:36

hedgar2017 requested changes Aug 20, 2026

View reviewed changes

abinavpp force-pushed the app-mlir-spill branch from 935e266 to db4030d Compare August 20, 2026 14:39

abinavpp marked this pull request as draft August 20, 2026 14:39

abinavpp force-pushed the app-mlir-spill branch 2 times, most recently from cf298a4 to c72be44 Compare August 21, 2026 15:50

abinavpp force-pushed the app-mlir-spill branch from c72be44 to eb850f7 Compare August 31, 2026 12:16

hedgar2017 force-pushed the main-slang branch 2 times, most recently from c049410 to 83d1c18 Compare August 31, 2026 12:30

abinavpp force-pushed the app-mlir-spill branch 3 times, most recently from 809a38f to 92f5806 Compare September 1, 2026 10:23

vladimirradosavljevic reviewed Sep 2, 2026

View reviewed changes

abinavpp force-pushed the app-mlir-spill branch 3 times, most recently from 3e3cf32 to 84c3d66 Compare September 4, 2026 08:42

abinavpp added 2 commits September 4, 2026 12:54

chore: bump solx-llvm for the memoryguard fold pass

1987094

The pipeline needs evm-fold-memory-guard, which the pinned revision predates.

abinavpp force-pushed the app-mlir-spill branch from 84c3d66 to c7393c1 Compare September 4, 2026 10:57

abinavpp mentioned this pull request Sep 4, 2026

fix(evm): describe the spill region with module flags #690

Open

abinavpp marked this pull request as ready for review September 4, 2026 13:38

hedgar2017 requested changes Sep 5, 2026

View reviewed changes

Uh oh!

Conversation

abinavpp commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Summary

Uh oh!

hedgar2017 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abinavpp commented Aug 20, 2026

Uh oh!

abinavpp commented Aug 20, 2026

Uh oh!

hedgar2017 left a comment

Choose a reason for hiding this comment

Uh oh!

vladimirradosavljevic Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

abinavpp commented Sep 4, 2026

Uh oh!

hedgar2017 Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

hedgar2017 Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

hedgar2017 Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

hedgar2017 Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

hedgar2017 Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

abinavpp commented Aug 16, 2026 •

edited

Loading

github-actions Bot commented Aug 16, 2026 •

edited

Loading

hedgar2017 left a comment •

edited

Loading