Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: debug mem2reg for load in loop #6978

Closed
wants to merge 1 commit into from

Conversation

asterite
Copy link
Collaborator

@asterite asterite commented Jan 7, 2025

Description

Problem

Related to #6971

Summary

This just adds an unfinished test with a small SSA snippet that I believe could be optimized out.

Next I'll check the mem2reg code to see if I can understand why it's not optimizing it.

Additional Context

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.

Copy link
Contributor

github-actions bot commented Jan 7, 2025

Compilation Report

Program Compilation Time %
sha256_regression 1.336s -2%
regression_4709 0.777s -2%
ram_blowup_regression 15.040s -1%
rollup-root 3.570s -15%
rollup-block-merge 4.310s 4%
rollup-base-public 29.600s -2%
rollup-base-private 12.200s -8%
private-kernel-tail 1.034s 0%
private-kernel-reset 6.800s -1%
private-kernel-inner 2.192s -1%

Copy link
Contributor

github-actions bot commented Jan 7, 2025

Execution Report

Program Execution Time %
sha256_regression 0.101s 1%
regression_4709 0.001s 0%
ram_blowup_regression 0.581s 0%
rollup-root 0.105s 0%
rollup-block-merge 0.106s 0%
rollup-base-public 1.430s -1%
rollup-base-private 0.648s 1%
private-kernel-tail 0.023s 0%
private-kernel-reset 0.389s 0%
private-kernel-inner 0.117s 0%

Copy link
Contributor

github-actions bot commented Jan 7, 2025

Compilation Memory Report

Program Peak Memory
keccak256 78.50M
workspace 123.72M
regression_4709 422.91M
ram_blowup_regression 1.58G
rollup-base-public 2.55G
rollup-base-private 1.25G
private-kernel-tail 201.97M
private-kernel-reset 716.57M
private-kernel-inner 292.25M

Copy link
Contributor

github-actions bot commented Jan 7, 2025

Execution Memory Report

Program Peak Memory
keccak256 74.61M
workspace 123.57M
regression_4709 315.93M
ram_blowup_regression 512.47M
rollup-base-public 773.33M
rollup-base-private 424.88M
private-kernel-tail 182.01M
private-kernel-reset 255.58M
private-kernel-inner 215.19M

@jfecher
Copy link
Contributor

jfecher commented Jan 7, 2025

@asterite you may be interested in some of my notes from the last time I looked at this problem: https://hackmd.io/@JakeF/S1FA9rTnh#Better-Loop-Optimizations. I thought I had a solution originally but it ended up being broken (only somewhat implicitly stated by the problems at the bottom being unfinished)

@asterite
Copy link
Collaborator Author

asterite commented Jan 7, 2025

@jfecher Interesting, thanks! I'll take a look. It seems I'm just finding this out when this was already known 😊 . That said, the first snippet:

unconstrained fn main() {
    let mut foo = 7;
    for i in 0 .. 10 {}
    assert_eq(foo, 7);
}

seems to be optimized out now.

@jfecher
Copy link
Contributor

jfecher commented Jan 7, 2025

@asterite we sometimes do loop unrolling in unconstrained code now, so you can "fix" that example to get it to not optimize by preventing loop unrolling. I found adding a println does the trick:

unconstrained fn main() {
    let mut foo = 7;
    for i in 0 .. 10 {
        println(i);
    }
    assert_eq(foo, 7);
}

Now in b2 before the constrain you should still see a load v1 that should be able to be optimized out, but isn't.

@asterite
Copy link
Collaborator Author

asterite commented Jan 7, 2025

Thanks! I'll close this PR as the solution seems to be a lot harder than what I thought.

@asterite asterite closed this Jan 7, 2025
@asterite asterite deleted the ab/mem2reg-load-in-loop branch January 7, 2025 20:48
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.

2 participants