Skip to content

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

Merged
fitzgen merged 1 commit intobytecodealliance:mainfrom
fitzgen:gvn-idempotent-traps
Jan 5, 2023
Merged

Cranelift: GVN all idempotently trapping but otherwise pure instructions#5534
fitzgen merged 1 commit intobytecodealliance:mainfrom
fitzgen:gvn-idempotent-traps

Conversation

@fitzgen
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

jameysharp commented Jan 6, 2023

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

1 more reviewer

@jameysharp jameysharp jameysharp approved these changes

Reviewers whose approvals may not affect merge requirements

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