Stream: git-wasmtime

Topic: wasmtime / PR #8761 WIP: cranelift/isle: add `Binding::Ma...

view this post on Zulip Wasmtime GitHub notifications bot (Jun 08 2024 at 22:06):

mmcloughlin opened PR #8761 from mmcloughlin:mbm/isle-binding-make-some to bytecodealliance:main:

Work-in-progress PR to demonstrate an idea for discussion.

When working with the ISLE "trie again" representation there is an awkward detail with partial internal constructors. Specifically, the result binding in the rule will refer to an expression of type T, while the return type of the generated constructor is type Option<T>. The wrapping of the return expression in Some(..) happens at codegen when the return kind is ReturnKind::Option.

This presents an incompatibility when implementing inlining at the trie level. If a constructor binding is selected for inlining then all the bindings for the rule application are imported, and the constructor callsite is replaced with the result binding. Therefore the result of inlining replaces a binding of type Option<T> with one of type T and the result is invalid. To fix this we just need to figure out where the right place is to insert the Some(..) wrapper.

This PR offers one approach to add a Binding::MakeSome variant and materialize the Some(..) wrapping in the trie. Note we already have Binding::{Match,Make}Variant and Binding::MatchSome so Binding::MakeSome is a natural addition. The rule set builder is expanded to wrap the result binding in MakeSome for partial constructors. With this change the Some(..) wrapper in codegen is no longer needed, and the inlining problem mentioned above goes away.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2024 at 01:04):

github-actions[bot] commented on PR #8761:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "isle"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jun 13 2024 at 03:47):

mmcloughlin edited PR #8761:

When working with ISLE's "trie again" representation there is an awkward detail with partial internal constructors. The result binding of a rule will refer to an expression of type T, while the return type of the generated constructor is type Option<T>. The wrapping of the return expression in Some(..) happens at codegen when the return kind is ReturnKind::Option.

This presents an incompatibility when implementing inlining at the trie level. If a constructor binding is selected for inlining then all the bindings for the rule application are imported, and the constructor callsite is replaced with the result binding. Therefore the result of inlining replaces a binding of type Option<T> with one of type T and the result is invalid. To fix this we just need to figure out where the right place is to insert the Some(..) wrapper.

This PR offers one approach to add a Binding::MakeSome variant and materialize the Some(..) wrapping in the trie. Note we already have Binding::{Match,Make}Variant and Binding::MatchSome so Binding::MakeSome is a natural addition. The rule set builder is expanded to wrap the result binding in MakeSome for partial constructors. With this change the Some(..) wrapper in codegen is no longer needed, and the inlining problem mentioned above goes away.

Last updated: Nov 22 2024 at 17:03 UTC