bytecodealliance / wasmtime

A fast and secure runtime for WebAssembly
https://wasmtime.dev/
Apache License 2.0
15.21k stars 1.28k forks source link

cranelift/isle: add `Binding::MakeSome` for partial constructors #8761

Closed mmcloughlin closed 3 months ago

mmcloughlin commented 3 months ago

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.

github-actions[bot] commented 3 months ago

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "cranelift", "isle" Thus the following users have been cc'd because of the following labels: * cfallin: isle * fitzgen: isle To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file. [Learn more.](https://github.com/bytecodealliance/subscribe-to-label-action)
mmcloughlin commented 3 months ago

cc @jameysharp