Stream: git-wasmtime

Topic: wasmtime / PR #7882 Guard recursion in `will_simplify_wit...


view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 00:58):

elliottt opened PR #7882 from elliottt:trevor/fix-7874 to bytecodealliance:main:

Add a test to expose issues with unbounded recursion through iadd during egraph rewrites, and bound the recursion of will_simplify_with_ireduce.

Fixes #7874

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 00:58):

elliottt has marked PR #7882 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 00:58):

elliottt requested wasmtime-compiler-reviewers for a review on PR #7882.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 00:58):

elliottt requested fitzgen for a review on PR #7882.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 00:58):

elliottt requested alexcrichton for a review on PR #7882.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 01:01):

elliottt edited PR #7882:

Expose the underlying issue of #7874 by adding a test with a long chain of additions, and then fix the bug by bounding the recursion of will_simplify_with_ireduce.

Reading through the changes in #7719, it seemed like the recursive cases in will_simplify_with_ireduce were missing two things:

  1. an additional guard of reducible_modular_op before the recursion, which showed up in the entry-point in simplify
  2. a depth bound to ensure that the recursion doesn't get out of control.

This PR makes both of these changes, and resolves the compilation time issues reported in #7874.

Fixes #7874

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 01:03):

elliottt edited PR #7882:

Expose the underlying issue of #7874 by adding a test with a long chain of additions, and then fix the bug by bounding the recursion of will_simplify_with_ireduce.

Reading through the changes in #7719, it seemed like the recursive cases in will_simplify_with_ireduce were missing two things:

  1. an additional guard of reducible_modular_op before the recursion, which showed up in the entry-point in simplify
  2. a depth bound to ensure that the recursion doesn't get out of control.

This PR makes both of these changes, and resolves the compilation time issues reported in #7874. The depth bound was arbitrary, and it would be fine to increase it if necessary.

Fixes #7874

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 02:44):

github-actions[bot] commented on PR #7882:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "isle"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 05:01):

fitzgen submitted PR review:

Looks great!

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 05:41):

fitzgen merged PR #7882.


Last updated: Dec 23 2024 at 12:05 UTC