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

Cranelift: remove non-egraphs optimization pipeline and use_egraphs option. #6167

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Apr 6, 2023

This PR removes the LICM, GVN, and preopt passes, and associated support pieces, from cranelift-codegen. Not to worry, we still have optimizations: the egraph framework subsumes all of these, and has been on by default since #5181.

A few decision points:

  • Filetests for the legacy LICM, GVN and simple_preopt were removed too. As we built optimizations in the egraph framework we wrote new tests for the equivalent functionality, and many of the old tests were testing specific behaviors in the old implementations that may not be relevant anymore. However if folks prefer I could take a different approach here and try to port over all of the tests.

  • The corresponding filetest modes (commands) were deleted too. The test alias_analysis mode remains, but no longer invokes a separate GVN first (since there is no separate GVN that will not also do alias analysis) so the tests were tweaked slightly to work with that. The egrpah testsuite also covers alias analysis.

  • The divconst_magic_numbers module is removed since it's unused without simple_preopt, though this is the one remaining optimization we still need to build in the egraphs framework, pending cranelift/egraphs: allow simplifying trapping arithmetic #5908. The magic numbers will live forever in git history so removing this in the meantime is not a major issue IMHO.

  • The use_egraphs setting itself was removed at both the Cranelift and Wasmtime levels. It has been marked deprecated for a few releases now (Wasmtime 6.0, 7.0, upcoming 8.0, and corresponding Cranelift versions) so I think this is probably OK. As an alternative if anyone feels strongly, we could leave the setting and make it a no-op.

… option.

This PR removes the LICM, GVN, and preopt passes, and associated support
pieces, from `cranelift-codegen`. Not to worry, we still have
optimizations: the egraph framework subsumes all of these, and has been
on by default since bytecodealliance#5181.

A few decision points:

- Filetests for the legacy LICM, GVN and simple_preopt were removed too.
  As we built optimizations in the egraph framework we wrote new tests
  for the equivalent functionality, and many of the old tests were
  testing specific behaviors in the old implementations that may not be
  relevant anymore. However if folks prefer I could take a different
  approach here and try to port over all of the tests.

- The corresponding filetest modes (commands) were deleted too. The
  `test alias_analysis` mode remains, but no longer invokes a separate
  GVN first (since there is no separate GVN that will not also do alias
  analysis) so the tests were tweaked slightly to work with that. The
  egrpah testsuite also covers alias analysis.

- The `divconst_magic_numbers` module is removed since it's unused
  without `simple_preopt`, though this is the one remaining optimization
  we still need to build in the egraphs framework, pending bytecodealliance#5908. The
  magic numbers will live forever in git history so removing this in the
  meantime is not a major issue IMHO.

- The `use_egraphs` setting itself was removed at both the Cranelift and
  Wasmtime levels. It has been marked deprecated for a few releases now
  (Wasmtime 6.0, 7.0, upcoming 8.0, and corresponding Cranelift
  versions) so I think this is probably OK. As an alternative if anyone
  feels strongly, we could leave the setting and make it a no-op.
@cfallin cfallin requested review from a team as code owners April 6, 2023 15:54
@cfallin cfallin requested review from elliottt, alexcrichton, fitzgen and jameysharp and removed request for a team April 6, 2023 15:54
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

🎊

Comment on lines -723 to -727
/// Fold comparisons into branch operations when possible.
///
/// This matches against operations which compare against zero, then use the
/// result in a conditional branch.
fn branch_opt(pos: &mut FuncCursor, inst: Inst) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we're still unable to port this optimization to egraphs. That shouldn't block this PR, but it would be nice to optimize control flow in the future as well :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes, it looks like this is doing (brif (icmp eq a 0) ...) -> (brif a ...) and the equivalent for ne -- that would indeed be useful to have someday!

Copy link
Contributor

Choose a reason for hiding this comment

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

Specifically, that issue is tracked in #6106.

Copy link
Member

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

Awesome! 🎉

@cfallin cfallin added this pull request to the merge queue Apr 6, 2023
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:meta Everything related to the meta-language. labels Apr 6, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 6, 2023
@cfallin cfallin added this pull request to the merge queue Apr 6, 2023
@cfallin
Copy link
Member Author

cfallin commented Apr 6, 2023

Test failure was a flake in wasi_tokio::poll_oneoff_files on macOS (cc @sunfishcode, is this still a known issue / can we do anything about it?)

Merged via the queue into bytecodealliance:main with commit 230e213 Apr 6, 2023
@cfallin cfallin deleted the remove-non-egraphs branch April 6, 2023 18:51
brendandburns pushed a commit to brendandburns/wasmtime that referenced this pull request Apr 13, 2023
… option. (bytecodealliance#6167)

* Cranelift: remove non-egraphs optimization pipeline and `use_egraphs` option.

This PR removes the LICM, GVN, and preopt passes, and associated support
pieces, from `cranelift-codegen`. Not to worry, we still have
optimizations: the egraph framework subsumes all of these, and has been
on by default since bytecodealliance#5181.

A few decision points:

- Filetests for the legacy LICM, GVN and simple_preopt were removed too.
  As we built optimizations in the egraph framework we wrote new tests
  for the equivalent functionality, and many of the old tests were
  testing specific behaviors in the old implementations that may not be
  relevant anymore. However if folks prefer I could take a different
  approach here and try to port over all of the tests.

- The corresponding filetest modes (commands) were deleted too. The
  `test alias_analysis` mode remains, but no longer invokes a separate
  GVN first (since there is no separate GVN that will not also do alias
  analysis) so the tests were tweaked slightly to work with that. The
  egrpah testsuite also covers alias analysis.

- The `divconst_magic_numbers` module is removed since it's unused
  without `simple_preopt`, though this is the one remaining optimization
  we still need to build in the egraphs framework, pending bytecodealliance#5908. The
  magic numbers will live forever in git history so removing this in the
  meantime is not a major issue IMHO.

- The `use_egraphs` setting itself was removed at both the Cranelift and
  Wasmtime levels. It has been marked deprecated for a few releases now
  (Wasmtime 6.0, 7.0, upcoming 8.0, and corresponding Cranelift
  versions) so I think this is probably OK. As an alternative if anyone
  feels strongly, we could leave the setting and make it a no-op.

* Update test outputs for remaining test differences.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants