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

Alternative version of TypeGeneralizing #6108

Draft
wants to merge 124 commits into
base: main
Choose a base branch
from
Draft

Alternative version of TypeGeneralizing #6108

wants to merge 124 commits into from

Conversation

kripken
Copy link
Member

@kripken kripken commented Nov 10, 2023

In offline discussion we thought it would be useful to compare the current
TypeGeneralizing impl with one that does not use the new optimization framework,
to get an idea for overheads etc.

This PR reimplements that pass on top of #6105, #6106, #6107, specifically it uses the
extracted subtyping discovery logic from Unsubtyping, and on top of that it builds a
graph and does a flow on it to find the generalized types.

The existing test has a new run line. Results look right to me, though a few are
different as this pass does look into unreachable code (and does not run DCE
internally).

Marking as draft as this is just for comparison purposes.

@kripken
Copy link
Member Author

kripken commented Nov 16, 2023

Updated after the new batch of tests landed for TypeGeneralizing.

This matches the output on almost all of them, except for

  • CallRef where I don't think we know what we want yet, so I didn't look into it.
  • ArrayCopy cases where one reference is null, which is unreachable code anyhow.

The code seems reasonable to me but it could be simplified more. In particular there are some obvious similarities to GUFA, so some kind of framework could be extracted to support them both.

@kripken kripken changed the base branch from alt.3 to main November 16, 2023 21:34
kripken added a commit that referenced this pull request Feb 20, 2024
This pulls out the subtype-exprs.h parts of #6108

These are NFC in the current codebase, but are fixes for that unlanded PR, and
another unrelated PR that will be opened shortly.
@kripken
Copy link
Member Author

kripken commented Jul 1, 2024

@tlively Just a reminder for both of us that if we plan to look into the new optimization framework again then we should discuss this PR. I think it shows shorter and faster code than the other version of TypeGeneralizing, which suggests the new framework could be improved before we go in depth to use it in more places.

@tlively
Copy link
Member

tlively commented Jul 1, 2024

Yes, agreed. Finishing this single optimization with and without the new framework, then comparing the code complexity and performance will be an important first step before using the new framework more broadly.

radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
This pulls out the subtype-exprs.h parts of WebAssembly#6108

These are NFC in the current codebase, but are fixes for that unlanded PR, and
another unrelated PR that will be opened shortly.
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