Skip to content

feat(ssa): Prune dead block parameters#8239

Merged
vezenovm merged 18 commits intomasterfrom
mv/prune-dead-params
Apr 28, 2025
Merged

feat(ssa): Prune dead block parameters#8239
vezenovm merged 18 commits intomasterfrom
mv/prune-dead-params

Conversation

@vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Apr 25, 2025

Description

Problem*

Resolves #8237, #8229

Summary*

We now remove dead block parameters and update the respective predecessor terminators. This was done as follows:

  1. Tracking unused parameters during DIE
  2. Returning a map of (BasicBlockId -> unused parameters) for a given function
  3. Return a map of (FunctionId -> map(BasicBlockId -> unused parameters)) for the given Ssa
  4. Now go through the map of unused parameters returned from DIE
  5. If a block has unused parameters remove those parameters (except for the entry block)
  6. For each predecessor of the block for which we removed parameters, update the terminator instruction of the predecessor to remove the corresponding arguments

Additional Context

Partially resolves #8233 except for the minimum inliner setting #8233 (comment). Perhaps we are ok with that though as ACIR cannot be run with a different inliner setting either way. So for the same inliner setting they do in fact still match.

I could add #8233 as a regression test and just override the minimum inliner setting if we do view this as matching execution.

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 25, 2025

Changes to Brillig bytecode sizes

Generated at commit: bcd09482c7c1808ac5a0279a755d92e7a775212f, compared to commit: 43bbaa59700621d0891125ab891fa10a8a63edc8

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
brillig_entry_points_regression_8069_inliner_max -4 ✅ -2.78%
brillig_entry_points_regression_8069_inliner_min -4 ✅ -2.78%
brillig_entry_points_regression_8069_inliner_zero -4 ✅ -2.78%

Full diff report 👇
Program Brillig opcodes (+/-) %
brillig_entry_points_regression_8069_inliner_max 140 (-4) -2.78%
brillig_entry_points_regression_8069_inliner_min 140 (-4) -2.78%
brillig_entry_points_regression_8069_inliner_zero 140 (-4) -2.78%

@github-actions
Copy link
Contributor

github-actions bot commented Apr 25, 2025

Changes to number of Brillig opcodes executed

Generated at commit: bcd09482c7c1808ac5a0279a755d92e7a775212f, compared to commit: 43bbaa59700621d0891125ab891fa10a8a63edc8

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
brillig_entry_points_regression_8069_inliner_max +27 ❌ +0.78%
brillig_entry_points_regression_8069_inliner_min +27 ❌ +0.78%
brillig_entry_points_regression_8069_inliner_zero +27 ❌ +0.78%

Full diff report 👇
Program Brillig opcodes (+/-) %
brillig_entry_points_regression_8069_inliner_max 3,497 (+27) +0.78%
brillig_entry_points_regression_8069_inliner_min 3,497 (+27) +0.78%
brillig_entry_points_regression_8069_inliner_zero 3,497 (+27) +0.78%

@vezenovm vezenovm requested a review from a team April 25, 2025 21:49
@vezenovm
Copy link
Contributor Author

Looks like the last failing test is in the debugger. The debugger is most likely keeping the bad overflowing instruction in use, thus we error out vs. normal execution. This PR should still be fine to review. I will add this as an exception to the debugger tests as we do in fact expect an error here with the debugger then.

@TomAFrench
Copy link
Member

Thoughts on just including this as part of the DIE pass?

@TomAFrench
Copy link
Member

This would also make it easier to trigger another round of DIE if necessary.

@vezenovm
Copy link
Contributor Author

vezenovm commented Apr 28, 2025

Thoughts on just including this as part of the DIE pass?

I wanted the pruning to be separate from the DIE logic, but you raise a good point that it is simpler to have them combined. I'll merge the passes (while still keeping the param pruning separate).

@TomAFrench
Copy link
Member

I think we can still keep it relatively separate but if we're always calling this immediately after DIE then it probably makes sense to have a "DIE+block params" pass which calls the two sub-passes internally. This also makes it easier for us to pass the state without having to pass it up into the dfg and down again.

@vezenovm
Copy link
Contributor Author

if we're always calling this immediately after DIE then it probably makes sense to have a "DIE+block params" pass which calls the two sub-passes internally. This also makes it easier for us to pass the state without having to pass it up into the dfg and down again.

Yeah I decided to go with turning DIE into a "DIE+block params" pass. However, for passing the state I still find it a bit cleaner to simply attach the unused parameters onto the BasicBlock itself. prune_dead_parameters doesn't need to accept any extra state as input then and Function::dead_instruction_elimination remains a bit simpler. DIE already returns some state for unused globals so perhaps we would prefer to have all state that DIE produces for follow-up passes to be returned and passed directly into the follow-up pass. Let me know what you prefer.

@TomAFrench
Copy link
Member

DIE already returns some state for unused globals so perhaps we would prefer to have all state that DIE produces for follow-up passes to be returned and passed directly into the follow-up pass.

I would prefer for us to pass this state around outside of the DFG as it prevents something which is now internal to this new SSA pass leaking into the global SSA state.

@vezenovm vezenovm enabled auto-merge April 28, 2025 20:36
@vezenovm vezenovm added this pull request to the merge queue Apr 28, 2025
Merged via the queue into master with commit 96e8679 Apr 28, 2025
114 checks passed
@vezenovm vezenovm deleted the mv/prune-dead-params branch April 28, 2025 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SSA: Remove dead block parameters and adjust terminators Only Brillig: attempted to subtract with overflow

3 participants