The new Yul optimizer toolkit#519
Conversation
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Implement type-inference-driven value narrowing in newyork codegen: 1. Let-binding narrowing: i256 values with inferred width ≤I64 and unsigned are truncated to i64 at Let binding sites, enabling native RISC-V arithmetic downstream. 2. Arithmetic dispatch: Add/Mul check inferred result width. If result fits in i64, use ensure_min_width for native ops. Sub always uses i256 to handle negative results correctly. 3. Pointer-site narrowing: Memory offsets/lengths narrowed from i256 to i64 at use sites (mstore, mload, codecopy, etc.). 4. Fix condition_stmts forward inference: The forward type inference pass was not visiting Let statements inside for-loop condition blocks, causing large constants (e.g., type(int256).min) to retain default I1 width and be incorrectly truncated. 5. Fix inliner for large contracts: Single-call functions larger than 40 IR nodes are now deferred to LLVM's inliner instead of being inlined at IR level. This prevents creating monolithic dispatcher functions that cause regressions on OpenZeppelin contracts. Results: OZ ERC20 -3.5%, OZ ERC721 -3.9%, small benchmarks -1.5% to -3.1%. All 62 integration tests, 5851 retester tests, and make test pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add a truncate-and-extend pattern after loading the free memory pointer (mload(64)) to prove to LLVM that the value fits in 64 bits. This enables LLVM to eliminate downstream overflow checks and simplify surrounding arithmetic, yielding significant code size reductions. Also adds revert(0,K) block deduplication using shared basic blocks keyed by constant revert length. Results vs standard pipeline: - Integration ERC20: -10.8% (was -5.1%) - Integration SHA1: -7.9% (was -1.4%) - OZ ERC20: -11.7% (74,508 vs 84,364 bytes) - OZ ERC721: -9.4% (84,950 vs 93,730 bytes) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Key changes: - FMP range proof changed from 64-bit to 32-bit to match XLEN, eliminating 60 more overflow trap sites in OZ ERC20 - Added apply_range_proof() helper for reusable range proofs - Added range proofs for timestamp (64), number (64), chainid (64), basefee (128) - Documented which builtins already have implicit range proofs - Memory region annotation in simplify pass using constant tracking - Updated IR_PLAN.md with optimization analysis and failed experiments Results: ERC20 -12.6%, Flipper -10.8%, Computation -10.5% vs standard. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cture Conditional sbrk NoInline: mark __sbrk_internal as NoInline only for contracts with >20 heap operations. ERC20 saves 359 bytes while small contracts remain unaffected. Interprocedural parameter narrowing: infrastructure to narrow function parameter types from I256 based on body-internal usage analysis. Currently no parameters meet the strict criteria but the code path is ready for broader narrowing rules. ERC20: 14287 bytes (-14.7% vs standard 16757) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The try_narrow_let_binding was overriding the 32-bit FMP range proof (trunc i256 → i32; zext i32 → i256) with a weaker 64-bit proof (trunc i256 → i64). Now detects zext source width and preserves tighter existing proofs. Result: 17 fewer overflow checks in LLVM IR, 288 fewer IR lines, though PVM blob size unchanged (PolkaVM already handles dead overflow blocks efficiently). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Major newyork IR optimizations: 1. Keccak256Pair IR node: fuses mstore(0,w0)+mstore(32,w1)+keccak256(0,64) into a single __revive_keccak256_two_words call. 13 sites in ERC20, saving ~529 bytes (-3.7%). 2. FMP constant propagation: replaces mload(0x40) with known constant when the free memory pointer hasn't been modified. Handles Statement::Block wrapping, Statement::Expr void calls, and builds transitive FMP-writer closure via call graph analysis. 3. Interprocedural parameter narrowing: propagates caller argument widths to callee parameters in the type inference forward pass. Narrows function signatures (e.g., assert_helper(i32), finalize_allocation(i64, i256)). 4. Conditional sbrk NoInline: only outline __sbrk_internal when the contract has >20 heap operations (saves ~359 bytes on ERC20 without regressing small contracts). 5. Load-after-store forwarding in mem_opt: when mload matches a tracked mstore to the same offset, forward the stored value directly. 6. Memory region annotation in simplify pass: tag MLoad/MStore with FreePointerSlot/Scratch/Dynamic regions from constant offset analysis. Code size improvements (newyork vs previous): - ERC20: 14287 → 13051 (-8.7%) - SHA1: 6748 → 5930 (-12.1%) - Computation: 1654 → 1577 (-4.7%) - DivisionArithmetics: 13501 → 13243 (-1.9%) All 62 integration tests pass with RESOLC_USE_NEWYORK=1. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two new code size optimizations:
1. PanicRevert IR node: detects the Solidity panic pattern
(mstore(0, 0x4e487b71...) + mstore(4, code) + revert(0, 0x24))
and replaces it with a PanicRevert { code } statement that lowers
to a shared helper function. Each panic code (0x11 overflow,
0x22 encoding, 0x41 memory, etc.) gets its own shared block,
deduplicating the 3-statement pattern across many call sites.
2. Shared return blocks: when multiple return(offset, length)
statements have identical constant arguments, they branch to a
single shared LLVM basic block instead of each generating their
own exit sequence (sbrk + ptrtoint + seal_return).
Code size improvements:
- ERC20: 13051 → 12681 (-2.8%)
- DivisionArithmetics: 13243 → 12950 (-2.2%)
All 62 integration tests pass with RESOLC_USE_NEWYORK=1.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1. Common subexpression elimination for pure environment reads (calldatasize, callvalue, caller, origin, address). After inlining, multiple switch cases redundantly call these builtins. CSE replaces 2nd+ occurrences with references to the first binding, enabling LLVM to eliminate redundant loads. 2. Storage load/store by-value functions: pass key and value as i256 directly instead of through alloca+pointer. Eliminates 2x alloca + 2x store overhead per storage operation. 3. Outlined caller_word function: moves the syscall + bswap + zext sequence into a shared function to avoid code duplication. 4. Fix clippy: remove useless .into() on BasicValueEnum arguments. Code size improvements: - ERC20: 12681 → 12386 (-2.3%) - Flipper: 1507 → 1493 (-0.9%) All 62 integration tests pass with RESOLC_USE_NEWYORK=1. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Earlier commits 549488e ("narrow finalize_allocation params via structural detection") and 06c39be ("extend allocator-shape detection to single-param size validators") narrowed both `p0` and `p1` of `finalize_allocation`, plus *any* I256 param whose entry guard was `if gt(p, UINT64_MAX) { panic }`. The panic guard is the part that breaks soundness: `new uint[](2 << 100)` is a valid Solidity expression that *intentionally* triggers panic 0x41 via that very check. Plain-truncating the i256 size argument at the call site silently drops the high bits before the guard observes them, so the wide value never trips the comparison and the panic is lost. The differential tests `complex/try_catch/call/panic/test.json` and `complex/try_catch/create/panic/test.json` (cases 7 and 16) cover this exact path; both regressed with the previous narrowing. Restrict Pattern 1 to narrowing only `p0` — that's the FMP-typed addend sourced from `mload(0x40)` in OZ contracts, bounded by heap_size, so plain truncation is sound. Drop Pattern 2 entirely; the body-internal AND-mask narrowing from the iter25/26 guard_narrow is still inserted, so the function body itself benefits from narrowed downstream arithmetic — just not via a narrowed signature. Verified: all 564 try_catch retester cases pass; clippy and fmt clean.
Add a structural rewrite that, for every `finalize_allocation`-shape
function (`(i256, i256) -> ()` whose body has the
`mstore(0x40, sum)` + `add(p0, _)` + `if or(gt(sum, UINT64_MAX),
lt(sum, p0)) panic` triple), prepends an explicit
`if gt(p1, UINT64_MAX) panic_0x41()` plus a downstream
`p1 = and(p1, UINT64_MAX)` rewrite. All subsequent uses of `p1` in
the body are redirected to the AND-masked value.
Soundness: the inserted gt/panic pair is *redundant* with the body's
existing overflow check — any caller that would have failed the early
check would also have failed the existing late one, so semantics are
preserved. The point is to expose the `p1 ≤ UINT64_MAX` invariant at
the *top* of the function so type-inference / LLVM IPSCCP can fold
the body's i256 arithmetic to i128 / i64. The function signature
remains `(i64, i256)`, so caller IR is unchanged — sidestepping the
12-test panic-0x41 regression that the iter34 signature-narrowing
attempt produced.
After this rewrite, LLVM lowers the body to:
%gt = icmp samesign ugt i256 %1, UINT64_MAX
br i1 %gt, label %panic_0x41, label %if_join
%2 = trunc nuw nsw i256 %1 to i128
; rest of body is i128 arithmetic
vs. the previous full-i256 body. Code-size impact across the OZ
contracts (vs iter33 baseline 364,534):
erc1155 -9
erc20 -8
erc721 -7
oz_gov -4
oz_rwa -24
oz_simple -33
oz_stable -122
proxy 0
Total -207 (-0.06%)
Verified: 70/70 integration tests, 49/49 newyork unit tests, 564/564
try_catch/{call,create}/panic retester cases (the iter34 regression
suite), and 674/674 wider sample (try_catch + create + defi +
array_one_element). Format and clippy clean.
Previously the post-MLoad range proof on `mload(0x40)` only fired when `can_use_native(0x40) || fmp_native_safe()` — i.e. when the contract was clean enough to use little-endian native storage. Most OZ contracts have dynamic memory accesses (array allocations, ABI copies) and so fail that gate, ending up with a 4-limb i256 FMP load + bswap + or chain at every `mload(0x40)` site. The native-safe check is about byte ORDER (InlineNative writes LE, ByteSwap reads BE — they must agree). The range bound is independent: sbrk enforces FMP < heap_size on every store regardless of mode, so the value loaded from 0x40 is always small. Removing the `can_use_native` gate from the range-proof condition gives LLVM a tight `<heap_size_bits>` range on every FMP load, which lets it DCE three of the four limb loads + bswaps + ors and the trailing `safe_truncate_int_to_xlen` overflow check. In oz_gov this drops the qword load count from 151 to 49. ### Code-size impact (vs iter35 baseline 364,327) | Contract | iter35 | now | Δ | |------------|---------|---------|----------| | erc1155 | 36,952 | 34,002 | -2,950 | | erc20 | 50,709 | 46,691 | -4,018 | | erc721 | 57,668 | 53,943 | -3,725 | | oz_gov | 97,989 | 86,973 | -11,016 | | oz_rwa | 48,144 | 43,386 | -4,758 | | oz_simple | 17,970 | 17,757 | -213 | | oz_stable | 50,605 | 46,276 | -4,329 | | proxy | 4,290 | 4,028 | -262 | | **Total** | 364,327 | 332,956 | **-31,371 (-8.6%)** | Cumulative since 80b14de (iter25 baseline 369,926): **-36,970 bytes (-9.99%)** across iter25-36. ### Tested - 70/70 integration tests, 49/49 newyork unit tests. - 564/564 try_catch retester cases (panic-0x41 path intact). - 1149/1149 broad sample (entire `complex/` directory). - Format and clippy clean.
The FMP slot at heap[0x40] holds a heap offset in `[0, heap_size)` — its maximum value is `heap_size - 1`, not `heap_size`. The previous range proof used `bits(heap_size)`, which for a power-of-two heap (e.g. 131072 = 2^17) is one bit looser than necessary. For heap_size = 131072: previously 18 bits (mask 0x3FFFF = 262143), now 17 bits (mask 0x1FFFF = 131071) — a single i32 `and` with a smaller immediate, and a tighter range hint propagating downstream. ### Code-size impact (vs iter36 baseline 332,956) | Contract | iter36 | now | Δ | |-----------|---------|---------|-----| | erc1155 | 34,002 | 34,002 | 0 | | erc20 | 46,691 | 46,684 | -7 | | erc721 | 53,943 | 53,943 | 0 | | oz_gov | 86,973 | 86,973 | 0 | | oz_rwa | 43,386 | 43,372 | -14 | | oz_simple | 17,757 | 17,751 | -6 | | oz_stable | 46,276 | 46,262 | -14 | | proxy | 4,028 | 4,028 | 0 | | **Total** | 332,956 | 332,915 | **-41** | Cumulative since iter25 baseline (369,926): **-37,011 bytes (-10.00%)** — crosses the 10% mark. ### Tested - 70/70 integration, 49/49 newyork unit - 564/564 try_catch panic retester - 674/674 wider differential sample - 16613/16613 simple/ retester (entire `simple/` corpus, 0 failed)
…raps The "Free memory pointer range proof" section was 2 lines. Expand it to capture (a) the trunc-N → zext-256 encoding pattern, (b) the multiplicative IPSCCP propagation effect that explains why one codegen site dominates the optimizer's overall code-size reduction, and (c) the byte-order vs value-bound separation that motivated dropping the `fmp_native_safe()` gate from the load-side range proof. Add a "Soundness traps for FMP optimizations" subsection listing the three previously-found bugs and their regression coverage: - mload_at_fmp_slot (1fd6063) — byte-order mismatch via dynamic mload - mload_huge_offset_traps (ccca38d) — memory-offset narrowing bypass - FMP i32 shortcut removal (dbcfc92) — short stores breaking inline asm Each tells future contributors what to verify when touching FMP code, and what the three independent invariants are (encoding, value bound, stored width).
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
| /// Environment variable: when set, dumps the newyork IR for every translated object to | ||
| /// `/tmp/newyork_ir_<object>.txt` after optimization passes have run. | ||
| pub const NEWYORK_DUMP_IR_ENV: &str = "NEWYORK_DUMP_IR"; |
There was a problem hiding this comment.
When debugging LLVM, I find it useful having the option to control after which specific pass to dump (e.g -print-after=<pass> instead of -print-after-all). We could consider adding something like that as well. It'd likely help debugging the optimizer.
There was a problem hiding this comment.
I also think we should consider CLI flags/arguments for this.
| pub struct SsaBuilder { | ||
| /// Next value ID to allocate. | ||
| next_value_id: u32, |
There was a problem hiding this comment.
Shouldn't SsaBuilder::next_value_id be of type ValueId rather than u32 directly? (Similarly in other places, such as InlineRemapper::next_value_id, some Simplifier fields, etc.)
| /// Defines a variable with the given name and value. | ||
| pub fn define(&mut self, name: &str, value: Value) { | ||
| self.current_scope.insert(name.to_string(), value); | ||
| } |
There was a problem hiding this comment.
define() currently handles both declarations and assignments. We could consider splitting the responsibility, e.g. into declare and assign, in order to catch some bugs earlier by adding appropriate assertions (e.g. asserting that the key does not already exist in the current scope, or that it does exist, etc.) (this is in addition to the current logic in validate.rs).
| /// Returns the scope that was exited (for computing modified variables). | ||
| pub fn exit_scope(&mut self) -> BTreeMap<String, Value> { | ||
| let exited = std::mem::take(&mut self.current_scope); | ||
| self.current_scope = self.scope_stack.pop().unwrap_or_default(); |
There was a problem hiding this comment.
Instead of unwrap_or_default(), shouldn't we use only unwrap() (or expect()), since a correct implementation should not call pop() on an empty stack.
| } | ||
|
|
||
| /// Allocates a fresh value ID. | ||
| pub fn fresh_value(&mut self) -> ValueId { |
There was a problem hiding this comment.
Nit: The equivalent method on Simplifier is called fresh_id. The latter is clearer to me.
| for error in &errors { | ||
| log::warn!("IR validation error in {}: {}", ir_object.name, error); | ||
| } |
There was a problem hiding this comment.
Why are validation failures logged at warn and not error?
| let leave_overhead = leave_count * leave_count * LEAVE_OVERHEAD_PER_SITE * call_count; | ||
| let cost = (call_count - 1) * size * CODE_SIZE_COST_MULTIPLIER + leave_overhead; | ||
| let mut benefit = 0; | ||
|
|
||
| if size <= 15 && leave_count == 0 { | ||
| benefit += SMALL_FUNCTION_BONUS; | ||
| } | ||
|
|
||
| if call_count <= 3 && leave_count <= 1 { | ||
| benefit += 10; | ||
| } |
There was a problem hiding this comment.
Could we instead use named constants for the magic numbers in this cost-benefit calculation?
| enum EnvRead { | ||
| CallDataSize, | ||
| CallValue, | ||
| Caller, | ||
| Origin, | ||
| Address, | ||
| } | ||
|
|
||
| /// Returns the `EnvRead` kind for an expression if it is a pure environment read. | ||
| fn env_read_kind(expression: &Expression) -> Option<EnvRead> { | ||
| match expression { | ||
| Expression::CallDataSize => Some(EnvRead::CallDataSize), | ||
| Expression::CallValue => Some(EnvRead::CallValue), | ||
| Expression::Caller => Some(EnvRead::Caller), | ||
| Expression::Origin => Some(EnvRead::Origin), | ||
| Expression::Address => Some(EnvRead::Address), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
Looking at the Expressions, there seem to be more possible values that could become an EnvRead. Don't e.g. the *Hash variants or ChainId or CodeSize also stay constant for the invocation?
| let mut redirects: BTreeMap<FunctionId, FunctionId> = BTreeMap::new(); | ||
|
|
||
| for function in object.functions.values() { | ||
| if function.size_estimate <= 3 { |
There was a problem hiding this comment.
Could we replace 3 with a constant instead. E.g. deduplicate_functions_fuzzy() uses if function.size_estimate < MIN_FUZZY_DEDUP_SIZE.
Late inline pass on top of the existing optimizer + cost-model retune + per-helper memory(...) attribute precision so GVN can fold across heap mstores. −8.95% on the OZ corpus vs the baseline. Co-authored-by: xermicus <bigcyrill@hotmail.com>
| let removed_count = redirects.len(); | ||
| redirect_calls_in_block(&mut object.code, &redirects); | ||
| for function in object.functions.values_mut() { | ||
| redirect_calls_in_block(&mut function.body, &redirects); | ||
| } | ||
|
|
||
| for dup_id in redirects.keys() { | ||
| object.functions.remove(dup_id); | ||
| } |
There was a problem hiding this comment.
When redirecting the calls and removing the duplicate functinons (similarly in deduplicate_functions_fuzzy()), I don't see any updates to the canonicalized function's call_count.
I assume this would currently not set the NoInline attribute in some scenarios after deduplication even if the actual call count is >= 3, based on LlvmCodegen::set_inline_attribtutes()'s InlineDecision::CostBenefit match arm:
revive/crates/newyork/src/to_llvm.rs
Lines 3477 to 3485 in 6ea5672
|
|
||
| /// Results of memory optimization. | ||
| #[derive(Clone, Debug, Default)] | ||
| pub struct MemOptResults { |
There was a problem hiding this comment.
MemOptResults doesn't actually seem to be used. I noticed that FmpPropagation::loads_eliminated only seemed to be incremented but not read, and when taking a closer look the same seemed to be true for the fields of MemOptResults.
| /// the constant 0x80 through them. | ||
| pub struct FmpPropagation { | ||
| /// Number of FMP loads eliminated. | ||
| pub loads_eliminated: usize, |
There was a problem hiding this comment.
loads_eliminated doesn't seem to be used (see previous comment).
| let is_fmp_store = | ||
| region == MemoryRegion::FreePointerSlot || resolved_offset == Some(0x40); |
There was a problem hiding this comment.
Why isn't just a resolved_offset == Some(0x40) check sufficient on its own? If the additional MemoryRegion::FreePointerSlot check is necessary, perhaps extracting the whole region == MemoryRegion::FreePointerSlot || resolved_offset == Some(0x40) into e.g. an is_fmp_slot() would be beneficial for maintenance and make it clearer as to why this check includes the || (the check is used both for is_fmp_store and is_fmp_load).
| if *region == MemoryRegion::Unknown && !pattern.is_aligned() { | ||
| if let Some(addr) = self.extract_static_offset(offset) { | ||
| if addr % 32 != 0 { | ||
| self.tainted_regions.insert(addr / 32 * 32); |
There was a problem hiding this comment.
The addr / 32 * 32 is used very frequently in this file (and a few times in mem_opt.rs as well), should we introduce a word address helper?
Fix various soundness issues at the cost of ~1% code size growth. Signed-off-by: xermicus <bigcyrill@hotmail.com>
Co-authored-by: xermicus <bigcyrill@hotmail.com>
Resolves conflicts after LLVM 21 → 22 upgrade and inkwell 0.8 → 0.9 landed on main: * Cargo.toml: keep bumped versions from main; preserve revive-newyork. * docs/: regenerated with mdbook against cl/newyork sources. * function/mod.rs: keep ours' NoFree+NoUnwind PVM target attributes; keep main's is_middle_end_enabled gate around NoInline/OptimizeNone. * context/mod.rs: drop narrow_divrem_instructions and strip_minsize_for_divrem — both were LLVM 21 backend workarounds removed by main in #501 once the upstream patch landed; LLVM 22 bundles the fix. * heap.rs: port inkwell 0.8 → 0.9 API breaks (custom_width_int_type now takes NonZeroU32 and returns Result). * codesize.json: kept ours (LLVM 22 bumps integration sizes ~5–60%; update intentionally in a follow-up). Verified: RESOLC_USE_NEWYORK=1 make test green (workspace + book). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds a per-operation reference for the newyork IR to the compiler book for easily being able to view printed syntax, operand and result types, examples, etc. for any operation. Using easily readable syntax notation. The new page is partly inspired by [MLIR's docs](https://mlir.llvm.org/docs/Dialects/SCFDialect/). <details> <summary>Example entry</summary> ### `and` (`Expression::Binary` with `BinaryOperation::And`) #### Description Bitwise AND. The common idiom for type narrowing: a constant mask on the right lets forward analysis pick up a tight result width. #### Syntax ```text and($lhs[: <type>], $rhs[: <type>]) ``` #### Example ```text let v2 := and(v0, v1) let v3 := and(v0, 0xff) // type inference narrows result to i8 ``` #### Operands | Name | Type | Notes | |---|---|---| | `lhs` | `i256` | — | | `rhs` | `i256` | — | #### Result and purity | Result | Purity | |---|---| | `min(width(lhs), width(rhs))` — AND can only clear bits, so the result fits in the narrower operand | Pure | #### Annotations None. </details>
Signed-off-by: xermicus <bigcyrill@hotmail.com>
* Drop the -O0 `optnone`/`noinline` gate from `set_default_attributes` (main #502). It pinned both attributes on every function when the middle end is off; with newyork-emitted IR (many small helpers expecting to be inlined) the backend's fast-isel produced PVM basic blocks over the 1000-instruction `BasicBlockTooLarge` limit. #502 itself reported the change as NFC on main, so the safeguard was inert there but actively harmful here. * Restore `narrow_divrem_instructions` after the LLVM pipeline. `strip_minsize_for_divrem` stays dropped — LLVM 22 carries the patch that made it unnecessary — but the narrowing safety net still helps i256 div/rem expand into compact backend code. * In `LlvmCodegen::set_inline_attributes`, early-return when the middle end is disabled and route the attribute write through `PolkaVMFunction::set_attributes(force = true)`. The raw `add_attribute` call stacked `alwaysinline` on top of the forced `noinline`/`optnone`, which the LLVM IR verifier rejected during library post-link compilation. `RESOLC_USE_NEWYORK=1 retester.sh`: 34888 passed, 6 failed, 144 ignored. The six failures are `blockhash(0xFFFF…)` at S- across all Y M{0,1,2,3,s,z} — a pre-existing newyork edge case masked at S+ by solc's optimizer folding the call. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Pre-existing newyork edge case: `blockhash(0xFFFF…)` reverts instead
of returning 0. solc's optimizer constant-folds the call to 0 at S+,
so the failure is only visible across Y M{0,1,2,3,s,z} at S-.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| if std::env::var(NEWYORK_DUMP_IR_ENV).is_ok() { | ||
| use std::io::Write; | ||
| let dump_path = format!("/tmp/newyork_ir_{}.txt", ir_object.name.replace('/', "_")); |
There was a problem hiding this comment.
IR dumps for NEWYORK_DUMP_IR seem to be hardcoded to "/tmp/newyork_ir_{}.txt". Could we use e.g. our current --debug-output-dir instead in order to make this configurable? (Similarly for RESOLC_DEBUG_*.)
There was a problem hiding this comment.
Currently, the IR dumps don't show the inferred widths at e.g. let bindings, operands, if-statement conditions, etc. even after the narrow pass has run (RESOLC_DEBUG_IR=1 dumps post-narrow). Some examples from compiling Flipper.sol:
let v19 := 0xffffffffffffffff
let v31 := 0x20
let v34 := slt(v7, v31)
if v34 { ... }
let v39 := iszero(v38)
let v49 := 0xff
let v50 := and(v39, v49)
I think it'd be more useful (and less ambiguous) if it showed the types in all cases, so it's easier to reason about if a type has been narrowed from looking at the IR. Something like:
let v19: i64 := 0xffffffffffffffff
let v31: i8 := 0x20
let v34: i1 := slt(v7: i64, v31: i8)
if v34: i1 { ... }
let v39: i1 := iszero(v38: i256)
let v49: i8 := 0xff
let v50: i64 := and(v39: i1, v49: i8) /* v50 may be i64 instead of i1 due to using a demanded width */
There was a problem hiding this comment.
Function calls in subobjects are printed as func_<id> rather than <func_name>. From compiling ERC20.sol:
let v141 := func_0(v140, v681) // should be abi_encode_string
let v154 := func_1(v689: i8) // should be abi_decode_address
let v248 := func_3(v247, v221) // should be checked_sub_uint256
let v270 := func_4(v269, v221) // should be checked_add_uint256
But sometimes there is a name printed, e.g. extract_byte_array_length() (in the dump after setting NEWYORK_DUMP_IR) which is the name of a function in the parent object, so there seems to be a FunctionId looked up from the wrong map (or populated incorrectly). Worth also looking into whether this bug goes beyond the printer.
| //! valid Yul but is designed to be human-readable and clearly show the SSA | ||
| //! structure with explicit value IDs. | ||
| //! | ||
| //! # Example Output |
There was a problem hiding this comment.
The doc comment's example output doesn't correspond to the actual current output (e.g. it uses = instead of :=).
| Statement::For { | ||
| initial_values, | ||
| loop_variables, | ||
| condition_statements, | ||
| condition, | ||
| body, | ||
| post, | ||
| outputs, | ||
| .. |
There was a problem hiding this comment.
The post_input_variables is dropped (..) here. (Also, the for-loop indentation is a little broken.)
| } => { | ||
| self.write_indent(); | ||
| let arg_strs: Vec<String> = | ||
| arguments.iter().map(|a| format!("v{}", a.id.0)).collect(); |
There was a problem hiding this comment.
This should probably use the existing write_value()/write_value_id().
No description provided.