Skip to content

fix modified in optimisation passes so they converge#7666

Merged
ironcev merged 14 commits into
masterfrom
xunilrj/fix-opt-passes-convergence
Jul 2, 2026
Merged

fix modified in optimisation passes so they converge#7666
ironcev merged 14 commits into
masterfrom
xunilrj/fix-opt-passes-convergence

Conversation

@xunilrj

@xunilrj xunilrj commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Description

This PR fix the modified return of some optimisation passes, so we can run optimisations multiple times until they converge.

To guarantee that they are correct, we now have an "env var" named "SWAY_VERIFY_FORCE" that forces IR verification and checks whether the modified flag is the same as the comparison of the IR before and after the pass. The flag is being set for all snapshot tests. As discussed, it makes sense to remove the "verify" CLI flag.

For this last check, I had to write the "mut" of function and block arguments in the IR; otherwise, the "mutability-tagger" pass would return true, but without any visible change in the IR, the check would always fail.

This triggered a LOT of changes, given that we assume everything is mutable by default. On the bright side is that it is now much easier to see improvements to the tagger pass. There are also other small improvements in the form of "TODO", because if Sway can guarantee that a value is never written, we can generate the initial IR already flagging something as immutable.

Another big change was the removal of "file-checks" for some IR tests. We already have snapshot tests for them, and given that the snapshot shows the complete diff, it is much easier to see the exact difference of each pass. I left normal comments though.

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 22, 2026 21:22 — with GitHub Actions Inactive
@codspeed-hq

codspeed-hq Bot commented Jun 22, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 25 untouched benchmarks


Comparing xunilrj/fix-opt-passes-convergence (5946264) with master (5a5b407)

Open in CodSpeed

@xunilrj

xunilrj commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

@cursor BugBot run

@cursor

cursor Bot commented Jun 23, 2026

Copy link
Copy Markdown

The PR already exists and doesn't need title/body changes. Let me provide my BugBot analysis summary:

BugBot Analysis of PR #7666: "fix modified in optimisation passes so they converge"

I reviewed the full diff (122 files, ~1600 lines changed) and found the following:

Bug fixed (committed & pushed):

  • Debug eprintln! with backtrace capture in sway-ir/src/analysis/memory_utils.rs — This was clearly leftover debugging code that would print a backtrace to stderr whenever pointee_size was called with a non-pointer type. Removed in commit 7faf90e04.

Minor observations (not bugs, but worth noting):

  • Value::replace always returns true — This is semantically imprecise (doesn't check if the new value differs from the old), but in practice all callers only invoke it when they know the value IS different, so it won't cause false convergence issues.
  • inline test: fold changed to any — This introduces short-circuit behavior for the "all" inline mode. Safe for all current tests (each has exactly 2 functions), but could silently skip inlining in future tests with 3+ functions that all have inlinable calls.

Overall assessment: The PR is well-structured. The core approach of making mutation functions (replace_values, remove_instruction, remove_instructions, remove_locals, remove_function) return bool is sound. The SWAY_VERIFY_FORCE env var for testing pass correctness is a good safety net. All CI checks pass.

Open in Web Open in Cursor 

@cursor

cursor Bot commented Jun 23, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches the compiler IR optimizer pipeline and pass convergence logic; incorrect modified reporting could cause missed optimizations or extra fixed-point iterations, but changes are heavily covered by updated IR snapshots and optional SWAY_VERIFY_FORCE checks.

Overview
Fixes optimization pass convergence by making passes report modified only when the IR actually changes, instead of often returning true unconditionally.

IR layer: replace_values, remove_instruction, remove_instructions, remove_locals, remove_function, and related helpers now return booleans that bubble up through DCE, conditional const-prop, mem2reg, memcpyopt, fn dedup, and arg mutability tagging so each pass aggregates real edits.

Pass manager: When SWAY_VERIFY_FORCE is set, every pass run is verified against a before/after IR string comparison and panics if the reported modified flag disagrees. IR tests lean on insta snapshots for Modified: true/false instead of requiring filecheck on every test.

IR text format: Block arguments can be parsed/printed with optional mut (parser + printer); many test .ir files drop redundant filecheck directives.

Smaller fixes: forc add compares workspace member vs package dirs via canonical paths to catch self-dependencies on macOS temp paths; assorted clippy/small refactors in forc-test, sway-core, and LSP sorting.

Reviewed by Cursor Bugbot for commit 7faf90e. Bugbot is set up for automated code reviews on this repo. Configure here.

@cursor cursor Bot temporarily deployed to fuel-sway-bot June 23, 2026 17:50 Inactive

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7faf90e. Configure here.

Comment thread forc-pkg/src/manifest/dep_modifier.rs Outdated
Comment thread sway-ir/src/value.rs
@xunilrj xunilrj force-pushed the xunilrj/fix-opt-passes-convergence branch from 7faf90e to a6a442f Compare June 23, 2026 18:14
@xunilrj xunilrj temporarily deployed to fuel-sway-bot June 23, 2026 18:14 — with GitHub Actions Inactive
Comment thread forc-test/src/execute.rs
Comment thread sway-core/src/asm_generation/fuel/fuel_asm_builder.rs
Comment thread sway-core/src/ir_generation/compile.rs
Comment thread sway-core/src/ir_generation/convert.rs
@xunilrj xunilrj temporarily deployed to fuel-sway-bot June 23, 2026 23:36 — with GitHub Actions Inactive
@xunilrj xunilrj temporarily deployed to fuel-sway-bot June 24, 2026 00:08 — with GitHub Actions Inactive
@xunilrj xunilrj marked this pull request as ready for review June 24, 2026 11:15
@xunilrj xunilrj requested review from a team as code owners June 24, 2026 11:15
@xunilrj xunilrj self-assigned this Jun 24, 2026
@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 labels Jun 27, 2026
@ironcev

ironcev commented Jun 27, 2026

Copy link
Copy Markdown
Member

👍

Comment thread sway-ir/tests/tests.rs

@ironcev ironcev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now when filecheck directives are deleted from the sway-ir tests, how will the failing tests actually be reported and fail the CI? By design we don't fail snapshot tests, so we will only get a message that a new snapshot was created, but the tests will not fail.

Comment thread sway-core/src/ir_generation/compile.rs
Comment thread sway-core/src/ir_generation/compile.rs Outdated
Comment thread sway-core/src/ir_generation/compile.rs Outdated
Comment thread sway-core/src/ir_generation/function.rs Outdated
Comment thread sway-core/src/ir_generation/function.rs Outdated
Comment thread sway-ir/src/pass_manager.rs Outdated
Comment thread sway-ir/src/pass_manager.rs Outdated
Comment thread sway-ir/src/pass_manager.rs Outdated
Comment thread sway-ir/src/pass_manager.rs Outdated
Comment thread sway-ir/src/pass_manager.rs Outdated
Comment thread sway-ir/src/parser.rs Outdated
@xunilrj xunilrj temporarily deployed to fuel-sway-bot June 30, 2026 19:13 — with GitHub Actions Inactive
@xunilrj

xunilrj commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Now when filecheck directives are deleted from the sway-ir tests, how will the failing tests actually be reported and fail the CI? #7654 (comment) we don't fail snapshot tests, so we will only get a message that a new snapshot was created, but the tests will not fail.

On CI we will receive a message stating that snapshots do not match, and the test will fail.

@xunilrj xunilrj force-pushed the xunilrj/fix-opt-passes-convergence branch from 7685e1d to 5946264 Compare June 30, 2026 23:56
@xunilrj xunilrj temporarily deployed to fuel-sway-bot June 30, 2026 23:56 — with GitHub Actions Inactive
@xunilrj xunilrj requested a review from ironcev July 1, 2026 00:10
@ironcev ironcev enabled auto-merge (squash) July 2, 2026 09:04
@ironcev ironcev merged commit 4c3bc6e into master Jul 2, 2026
42 checks passed
@ironcev ironcev deleted the xunilrj/fix-opt-passes-convergence branch July 2, 2026 09:05
@ironcev

ironcev commented Jul 2, 2026

Copy link
Copy Markdown
Member

On CI we will receive a message stating that snapshots do not match, and the test will fail.

@xunilrj Can you please explain in more detail or maybe add a comment to CI in a follow up PR. Because what I see on CI is just a regular cargo test that will print green on all the 19 sway-ir tests even if there is an insta line printed between the tests. I don't see why would CI run of that cargo test differ from the local run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla:signed compiler: ir IRgen and sway-ir including optimization passes compiler General compiler. Should eventually become more specific as the issue is triaged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants