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

Add J2CL optimization pass to binaryen. #6151

Merged
merged 17 commits into from
Dec 12, 2023
Merged

Add J2CL optimization pass to binaryen. #6151

merged 17 commits into from
Dec 12, 2023

Conversation

gkdn
Copy link
Contributor

@gkdn gkdn commented Dec 7, 2023

This PR creates a new pass to optimize J2CL specific patterns that would otherwise difficult to recognize/prove generically by other binaryen passes.

The pass currently handles fields what we call as "constant-like". These fields are fields initialized once and unconditionally through "clinit" function and technically they do have 2 observable states;

  • initial null/0 state
  • initialized state. However you can only observe initial null/0 state in contrived examples, not in real world/correct applications.

This pass moves such "clinit" initialized fields to global initialization.

Above pattern also matches other lazy init construct like String and Class literals (which binaryen already reduces to constant expressions). So the pass is generalized to include them as well. (by matching any functions with the name pattern _@once_)

In order for this pass to be effective:

  1. It needs to run between O3 passes
  2. We need to stop inlining of "once" functions.

Stopping inlining of the once functions are important to preseve their structure. This both helps existing OnceReducer pass and new J2CL pass to be a lot more effective. Also it is not useful to inline these functions as by defintion they only executed once. This could be achieved by passing no-inline filter.

Although the inlining is generally disabled for these functions, it is still needed for some cases since inliner is effectively responsible for removal of the once functions that are simplified into empty or simple delegating functions. For this reason, the pass will rename such trivial function so no-inline filter will no longer match them.

Also note that after all optimizations completed, it does make sense to have a final stage where the "partial inline" of all once functions are allowed. This will speed them up by moving the initialization check to call-site.

This PR creates a new pass to optimize J2CL specific patterns
that would otherwise difficult to recognize/prove generically
by other binaryen passes.

The pass currently handles fields what we call as "constant-like".
These fields are fields initialized once and unconditionally through
"clinit" function and technically they do have 2 observable states;
 - initial null/0 state
 - initialized state.
However you can only observe initial null/0 state in contrived examples,
not in real world/correct applications.

This pass moves such "clinit" initialized fields to global initialization.

Above pattern also matches other lazy init construct like String and Class
literals (which binaryen already reduces to constant expressions). So
the pass is generalized to include them as well. (by matching any functions
with the name pattern "_@once_")

In order for this pass to be effective:
1. It needs to run between O3 passes
2. We need to stop inlining of "once" functions.

Stopping inlining of the once functions are important to preseve their
structure. This both helps existing OnceReducer pass and new J2CL pass to
be a lot more effective. Also it is not useful to inline these functions
as by defintion they only executed once. This could be achieved by passing
no-inline filter.

Although the inlining is generally disabled for these functions, it is
still needed for some cases since inliner is effectively responsible for
removal of the once functions that are simplified into empty or simple
delegating functions. For this reason, the pass will rename such trivial
function so no-inline filter will no longer match them.

Also note that after all optimizations completed, it does make sense to
have a final stage where the "partial inline" of all once functions are
allowed. This will speed them up by moving the initialization check to
call-site.
@gkdn
Copy link
Contributor Author

gkdn commented Dec 7, 2023

@kripken Again pardon my C++; I'm not familiar with best practices and guidelines so you can assume I don't know what I'm doing and comment accordingly. I will send the tests in follow up commits within same PR.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

In general looks good, just minor comments, and testing.

src/passes/CMakeLists.txt Outdated Show resolved Hide resolved
src/passes/J2clOpts.cpp Outdated Show resolved Hide resolved
src/passes/J2clOpts.cpp Outdated Show resolved Hide resolved
src/passes/J2clOpts.cpp Outdated Show resolved Hide resolved
src/passes/J2clOpts.cpp Outdated Show resolved Hide resolved
src/passes/J2clOpts.cpp Outdated Show resolved Hide resolved

if (!GlobalUtils::canInitializeGlobal(*getModule(), set->value)) {
// It is not a valid constant expression so cannot be hoisted to
// global init.
Copy link
Member

Choose a reason for hiding this comment

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

It is convenient that this rules out all the dangerous situations, like local state, doing a call, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, it was nicely fit!

src/passes/J2clOpts.cpp Outdated Show resolved Hide resolved
src/passes/J2clOpts.cpp Outdated Show resolved Hide resolved
src/passes/pass.cpp Outdated Show resolved Hide resolved
@gkdn
Copy link
Contributor Author

gkdn commented Dec 8, 2023

In general looks good, just minor comments, and testing.

Are there some instructions that I can follow to add the tests?

@kripken
Copy link
Member

kripken commented Dec 8, 2023

For testing, you can add a file in parallel to this one:

https://github.com/WebAssembly/binaryen/blob/main/test/lit/passes/inlining_splitting_basics.wast

The name can be the same as the commandline flag to run the pass.

To test it locally for quick iteration, you can run

./scripts/update_lit_checks.py test/lit/passes/j2cl.wast

(or whatever the filename ends up being). That adds the expected results in the file.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Code lgtm.

src/passes/J2clOpts.cpp Outdated Show resolved Hide resolved
@kripken
Copy link
Member

kripken commented Dec 8, 2023

You will also need to run ./scripts/update_help_checks.py once, to update the --help outputs (which will now mention the new pass).

@gkdn
Copy link
Contributor Author

gkdn commented Dec 9, 2023

@kripken Some of the expected results are put in weird places like:

module (
  ...
  (func $non-initial-value_@once@_
    (global.set $field-i32-non-initial (i32.const 1))
    (global.set $field-any-non-initial (struct.new $A))
  )
)

;; CHECK:      (func $initial-value (type $1)
;; CHECK-NEXT:  (nop)
;; CHECK-NEXT: )

;; CHECK:      (func $non-initial-value (type $1)
;; CHECK-NEXT:  (nop)
;; CHECK-NEXT: )

I'm not sure what I'm doing wrong.

@kripken
Copy link
Member

kripken commented Dec 11, 2023

I've seen it move expected outputs around when it doesn't find the corresponding input. Are those functions at the end new ones created by split inlining or such? That could explain it. Anyhow, it's probably not something wrong that you are doing, but I can take a closer look when the tests are added.

@gkdn
Copy link
Contributor Author

gkdn commented Dec 11, 2023

Oh I see. That's probably due to the renaming of the methods then (to enable inlining). I will stop doing that so should be fine.

@gkdn
Copy link
Contributor Author

gkdn commented Dec 11, 2023

PTAL @kripken

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

lgtm % comments

test/lit/passes/j2cl-inline.wast Outdated Show resolved Hide resolved
test/lit/passes/j2cl.wast Outdated Show resolved Hide resolved
test/lit/passes/j2cl.wast Outdated Show resolved Hide resolved
test/lit/passes/j2cl.wast Outdated Show resolved Hide resolved
gkdn and others added 5 commits December 11, 2023 16:56
Co-authored-by: Alon Zakai <alonzakai@gmail.com>
Co-authored-by: Alon Zakai <alonzakai@gmail.com>
Co-authored-by: Alon Zakai <alonzakai@gmail.com>
@gkdn
Copy link
Contributor Author

gkdn commented Dec 12, 2023

I tihnk it is good to go now.

@kripken kripken merged commit 71dad87 into WebAssembly:main Dec 12, 2023
13 of 14 checks passed
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
This PR creates a new pass to optimize J2CL specific patterns
that would otherwise difficult to recognize/prove generically
by other binaryen passes.

The pass currently handles fields what we call as "constant-like".
These fields are fields initialized once and unconditionally through
"clinit" function and technically they do have 2 observable states;
 - initial null/0 state
 - initialized state.
However you can only observe initial null/0 state in contrived examples,
not in real world/correct applications.

This pass moves such "clinit" initialized fields to global initialization.

Above pattern also matches other lazy init construct like String and Class
literals (which binaryen already reduces to constant expressions). So
the pass is generalized to include them as well. (by matching any functions
with the name pattern "_@once_")

In order for this pass to be effective:
1. It needs to run between O3 passes
2. We need to stop inlining of "once" functions.

Stopping inlining of the once functions are important to preserve their
structure. This both helps existing OnceReducer pass and new J2CL pass to
be a lot more effective. Also it is not useful to inline these functions
as by defintion they only executed once. This could be achieved by passing
no-inline filter.

Although the inlining is generally disabled for these functions, it is
still needed for some cases since inliner is effectively responsible for
removal of the once functions that are simplified into empty or simple
delegating functions. For this reason, the pass will rename such trivial
function so no-inline filter will no longer match them.

Also note that after all optimizations completed, it does make sense to
have a final stage where the "partial inline" of all once functions are
allowed. This will speed them up by moving the initialization check to
call-site.
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