Skip to content

Improve raw_ptr::write#7646

Draft
xunilrj wants to merge 42 commits into
masterfrom
xunilrj/improve-rawptr-write
Draft

Improve raw_ptr::write#7646
xunilrj wants to merge 42 commits into
masterfrom
xunilrj/improve-rawptr-write

Conversation

@xunilrj

@xunilrj xunilrj commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Description

This PR is the first of a series trying to make raw_ptr::write optimal. For now, we tackle the issue that it was not being inlined.

The issue is that we only check for inlining at certain points, and some optimisations were being run after this check, ignoring the inlining even when it made sense.

The solution is to run the optimisation pipeline multiple times, a standard practice. For that, I had to fix other passes that were returning true for modified even when they do not change anything. This allows us to break the loop as early as possible.

Compilation time impact remains to be checked, but we can remove the loop on debug builds if that presents itself as a problem.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@xunilrj xunilrj temporarily deployed to fuel-sway-bot June 5, 2026 18:26 — with GitHub Actions Inactive
@xunilrj xunilrj changed the title Xunilrj/improve rawptr write Improve raw_ptr::write (1/n) Jun 5, 2026
@xunilrj

xunilrj commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

e2e-gas-usage

Improvements Regressions
Count 1038 51
Average 2.48% -0.92%
Median 0.52% -0.28%
Max 94.39% -5.59%
Min 0.10% -0.01%

e2e-bytecode-size

Improvements Regressions
Count 130 25
Average 11.80% -4.73%
Median 5.44% -3.17%
Max 97.61% -18.34%
Min 0.13% -0.16%

o2-bytecode-size

Improvements Regressions
Count 3 2
Average 2.94% -0.59%
Median 3.30% -0.59%
Max 3.55% -0.92%
Min 1.97% -0.26%

o2-gas-usage

Improvements Regressions
Count 1 1
Average 25.16% -0.42%
Median 25.16% -0.42%
Max 25.16% -0.42%
Min 25.16% -0.42%
Test Before After Percentage
contract_libs::test_u56 480 482 -0.42%
src20-token::storage_sizes_of_stored_types 640 479 25.16%

@xunilrj xunilrj temporarily deployed to fuel-sway-bot June 5, 2026 22:08 — with GitHub Actions Inactive
@codspeed-hq

codspeed-hq Bot commented Jun 5, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 20.32%

❌ 1 regressed benchmark
✅ 24 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
tokens_for_program 520.2 µs 652.8 µs -20.32%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing xunilrj/improve-rawptr-write (7196d95) with master (52671eb)

Open in CodSpeed

@ironcev ironcev added compiler General compiler. Should eventually become more specific as the issue is triaged compiler: ir IRgen and sway-ir including optimization passes performance Everything related to performance, speed wise or memory wise. labels Jun 8, 2026
@ironcev

ironcev commented Jun 8, 2026

Copy link
Copy Markdown
Member

👍

@xunilrj xunilrj temporarily deployed to fuel-sway-bot June 10, 2026 00:44 — with GitHub Actions Inactive
@xunilrj xunilrj temporarily deployed to fuel-sway-bot June 10, 2026 23:55 — with GitHub Actions Inactive
@xunilrj xunilrj temporarily deployed to fuel-sway-bot June 11, 2026 10:52 — with GitHub Actions Inactive
@xunilrj xunilrj temporarily deployed to fuel-sway-bot June 11, 2026 12:09 — with GitHub Actions Inactive
@xunilrj xunilrj temporarily deployed to fuel-sway-bot June 11, 2026 14:45 — with GitHub Actions Inactive
@xunilrj xunilrj mentioned this pull request Jun 11, 2026
8 tasks
ironcev added a commit that referenced this pull request Jun 11, 2026
## Description

This PR is a necessary change for
#7646.

Some IR tests have a bug: their harness is expecting some passes to
return `modified = true` even when the pass is not modifying anything.
To make sure that #7646 is NOT changing the optimisation itself, but
only fixing the `modified` flag, I am creating this PR.

The idea is that #7646 snapshot tests will have only the flag changed,
not their optimisation.

## Checklist

- [ ] I have linked to any relevant issues.
- [ ] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] If my change requires substantial documentation changes, I have
[requested support from the DevRel
team](https://github.com/FuelLabs/devrel-requests/issues/new/choose)
- [ ] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [ ] I have requested a review from the relevant team or maintainers.

---------

Co-authored-by: Igor Rončević <ironcev@hotmail.com>
@xunilrj xunilrj force-pushed the xunilrj/improve-rawptr-write branch from 5d5a052 to 7196d95 Compare June 12, 2026 12:10
@xunilrj xunilrj temporarily deployed to fuel-sway-bot June 12, 2026 12:10 — with GitHub Actions Inactive
xunilrj added a commit that referenced this pull request Jun 17, 2026
## Description

This PR is another prelude to
#7646.

We discovered that `memcpyopt` does not work when we run it multiple
times. The cause is that we bail the optimisation when a symbol has more
than one write. This is correct, but we were only considering 'store'.

Now we are correctly also considering all other ways to write into
"symbols".

## Checklist

- [ ] I have linked to any relevant issues.
- [ ] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] If my change requires substantial documentation changes, I have
[requested support from the DevRel
team](https://github.com/FuelLabs/devrel-requests/issues/new/choose)
- [ ] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [ ] I have requested a review from the relevant team or maintainers.
@xunilrj xunilrj force-pushed the xunilrj/improve-rawptr-write branch from 7196d95 to d591444 Compare July 8, 2026 18:22
@xunilrj xunilrj temporarily deployed to fuel-sway-bot July 8, 2026 18:22 — with GitHub Actions Inactive
@xunilrj xunilrj temporarily deployed to fuel-sway-bot July 11, 2026 19:11 — with GitHub Actions Inactive
@xunilrj xunilrj temporarily deployed to fuel-sway-bot July 11, 2026 19:21 — with GitHub Actions Inactive
@xunilrj

xunilrj commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

@cursor BugBot run

@cursor

cursor Bot commented Jul 12, 2026

Copy link
Copy Markdown

Here's a summary of the BugBot review I posted on PR #7646:

No critical bugs found. The core algorithms (dominator tree refactoring, loop analysis, pass manager loop) are correct. I identified 3 issues:

  • Medium — Iteration limit mismatch: run() loops up to 10 times but run_with_print_verify() (the production path used by the compiler) only loops up to 2. This may leave optimizations on the table if more iterations are needed.
  • Low — todo!() in instruction_print: The refactored printer panics for values that are neither instructions nor constants, rather than handling them gracefully.
  • Low — Inline heuristic behavioral change: The call-count check changed from == 1 to <= 1, which now accepts a count of 0 via unwrap_or(0). Harmless in practice but a subtle semantic shift.

I also noted a few minor observations: an unnecessary per-iteration allocation in run_with_print_verify, the loop analysis predecessor walk potentially including unreachable blocks, and the new LoopAnalysis pass being registered but not yet wired into any consumer (intentional for this "1/n" PR).

Open in Web Open in Cursor 

@cursor

cursor Bot commented Jul 12, 2026

Copy link
Copy Markdown

BugBot Review

I analyzed the full diff of this PR. Here are the findings:


Bug 1 (Medium): Inconsistent iteration limits between run() and run_with_print_verify()

File: sway-ir/src/pass_manager.rs

run() loops up to 10 times (line 373), but run_with_print_verify() loops up to only 2 times (line 414). The production compiler uses run_with_print_verify() (called from sway-core/src/lib.rs:1619), so the main compilation path gets at most 2 optimization iterations while the simpler run() path (used in tests and the opt binary) gets up to 10.

If the purpose of this PR is to run the pipeline multiple times until stabilization, limiting the production path to 2 may leave optimizations on the table. Conversely, if 2 is sufficient for convergence, the run() path is doing up to 5x unnecessary work.

Recommendation: Unify the iteration limit, or extract it into a shared constant with a clear rationale for any intentional difference.


Bug 2 (Low): todo!() panic in instruction_print

File: sway-ir/src/printer.rs, line 209

} else {
    todo!();
}

The refactored instruction_print handles instructions and constants but panics with todo!() for any other Value variant. While the function comment says "only used for debugging the compiler," a todo!() will crash the compiler if hit. Consider replacing with a more graceful fallback (e.g., write!(s, "<unknown value>")).


Bug 3 (Low): Subtle behavioral change in inline heuristic — == 1 to <= 1

File: sway-ir/src/optimize/inline.rs, line 149

The condition changed from:

if call_counts.get(func).copied().unwrap_or(0) == 1 {

to:

if call_counts.get(func).copied().unwrap_or(0) <= MAX_CALLS_COUNT_TO_INLINE {

With MAX_CALLS_COUNT_TO_INLINE = 1 and unwrap_or(0), this now returns true for functions with a call count of 0 (i.e., not found in call_counts). In practice, the predicate is only evaluated for functions that appear as callees in Call instructions, so call_counts should always have an entry. However, the <= 1 vs == 1 semantic difference is worth a comment or an explicit >= 1 && guard.


Observation 1: flatten_pass_group allocation per iteration in run_with_print_verify

File: sway-ir/src/pass_manager.rs

In run(), passes.flatten_pass_group() is called once and the Vec is reused across iterations (line 369). In run_with_print_verify(), it's called inside the loop body (line 417), allocating a new Vec on every iteration. Minor inefficiency — could be hoisted out of the loop.


Observation 2: Loop analysis predecessor walk may include unreachable blocks

File: sway-ir/src/analysis/loop_analysis.rs, line 127

q.extend(block.pred_iter(context));

When collecting loop body blocks via backward BFS from the loop tail, pred_iter may return unreachable predecessors (blocks not in the post-order). These would be added to loop_blocks and block_to_loops even though they're not part of the reachable CFG. This won't cause incorrect loop detection (back-edges are only found among reachable blocks), but it could make is_inside_loop return true for unreachable blocks. If unreachable blocks are cleaned up before loop analysis runs, this is a non-issue.


Observation 3: New loop analysis pass is registered but not wired into inline

File: sway-ir/src/optimize/inline.rs, line 30

The loop analysis dependency is commented out:

deps: vec![/*LOOP_ANALYSIS_NAME*/],

And the code that would use it to adjust thresholds is also commented out (lines 158-165). This is clearly intentional for this "1/n" PR, but worth noting that the LoopAnalysis infrastructure added here has no consumers yet.


Summary

# Severity Description
1 Medium Iteration limit mismatch: run() = 10, run_with_print_verify() = 2
2 Low todo!() panic in instruction_print for non-instruction/non-constant values
3 Low Inline heuristic <= 1 now accepts call count 0 (was == 1)

No critical bugs found. The core algorithms (dominator tree, loop analysis, pass manager loop) are correct.

@ironcev ironcev changed the title Improve raw_ptr::write (1/n) Improve raw_ptr::write Jul 13, 2026
@xunilrj

xunilrj commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Bug 1 (Medium): Inconsistent iteration limits between run() and run_with_print_verify()

Fixed. They are now the same value.

Bug 2 (Low): todo!() panic in instruction_print

This is intentional. This function is only used for debug and it is incomplete.

Bug 3 (Low): Subtle behavioral change in inline heuristic — == 1 to <= 1

This is fine. A call count of 0 would never be inlined anyways. The "<=" is needed if the constant is greater than one.

Observation 1: flatten_pass_group allocation per iteration in run_with_print_verify

Done.

Observation 2: Loop analysis predecessor walk may include unreachable blocks

Done

Observation 3: New loop analysis pass is registered but not wired into inline

Intentional. This will be used later.

@xunilrj xunilrj force-pushed the xunilrj/improve-rawptr-write branch from 8c9756f to a269d74 Compare July 13, 2026 19:38
@xunilrj xunilrj temporarily deployed to fuel-sway-bot July 13, 2026 19:39 — with GitHub Actions Inactive
@xunilrj xunilrj temporarily deployed to fuel-sway-bot July 14, 2026 10:41 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compiler: ir IRgen and sway-ir including optimization passes compiler General compiler. Should eventually become more specific as the issue is triaged performance Everything related to performance, speed wise or memory wise.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants