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.

Sign up for GitHub

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

Jump to bottom

Cranelift: GVN all idempotently trapping but otherwise pure instructions #5534

Merged
merged 1 commit into from
Jan 5, 2023
Merged

Cranelift: GVN all idempotently trapping but otherwise pure instructions #5534

merged 1 commit into from
Jan 5, 2023

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Jan 5, 2023

No description provided.

@fitzgen fitzgen requested a review from jameysharp January 5, 2023 21:35
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:meta Everything related to the meta-language. labels Jan 5, 2023
jameysharp
jameysharp approved these changes Jan 5, 2023
View reviewed changes
Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

Makes sense to me! I missed the introduction of side_effects_idempotent so I'm not sure if the egraph case handles it too or if it's only simple-gvn. But I think this should be safe to merge either way.

@fitzgen
Copy link
Member Author

fitzgen commented Jan 5, 2023

Makes sense to me! I missed the introduction of side_effects_idempotent so I'm not sure if the egraph case handles it too or if it's only simple-gvn. But I think this should be safe to merge either way.

It is just simple_gvn for now, but shouldn't be hard to introduce to the egraphs stuff since it's just a predicate on the opcode, and not some deep logic that needs to be copied over. I didn't implement it just because I don't know exactly where that would go yet. Maybe you do?

@fitzgen fitzgen merged commit c50bdf6 into bytecodealliance:main Jan 5, 2023
@fitzgen fitzgen deleted the gvn-idempotent-traps branch January 5, 2023 23:08
@jameysharp
Copy link
Contributor

I think the place to use side_effects_idempotent for egraph optimization is in is_pure_for_egraph, defined in cranelift/codegen/src/inst_predicates.rs. I'm not absolutely sure though.

cfallin added a commit to cfallin/wasmtime that referenced this pull request Jan 18, 2023
@cfallin
Update egraph-pure predicate as per GVN updates in bytecodealliance#5534
e0a2ce2 
.
cfallin added a commit to cfallin/wasmtime that referenced this pull request Jan 18, 2023
@cfallin
Support mergeable-but-side-effectful (idempotent) operations in gener…
35de219 
…al in the egraph's GVN.

This mirrors the similar change made in bytecodealliance#5534.
@cfallin cfallin mentioned this pull request Jan 18, 2023
Merged
cfallin added a commit to cfallin/wasmtime that referenced this pull request Jan 19, 2023
@cfallin
Support mergeable-but-side-effectful (idempotent) operations in gener…
b9d9719 
…al in the egraph's GVN.

This mirrors the similar change made in bytecodealliance#5534.
@cfallin cfallin mentioned this pull request Jan 19, 2023
Merged
cfallin added a commit that referenced this pull request Jan 19, 2023
@cfallin
Cranelift/egraph mid-end: support merging effectful-but-idempotent ops (
704f5a5 
#5594)

* Support mergeable-but-side-effectful (idempotent) operations in general in the egraph's GVN.

This mirrors the similar change made in #5534.

* Add tests for egraph case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jameysharp jameysharp jameysharp approved these changes

Assignees
No one assigned
Labels
cranelift:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fitzgen @jameysharp