search

bytecodealliance / wasmtime

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

ISLE: support bool types and constants properly #3573

Open cfallin opened 2 years ago

cfallin commented 2 years ago

ISLE's prototype compiler was built with the simplifying assumption that all constants are integers. "Symbolic constants" (imported opaque values) like $MYVALUE were added later. It's possible to define bool as a primitive type, but the syntax for bool constants, #t and #f, results in generated code that uses integer values 1 and 0 because... everything is (was) an integer. This is clearly suboptimal! In #3572 @alexcrichton used the clever workaround of $false, leveraging the support for arbitrary passthrough of constant names to actually get a false in the code.

We should (i) add a ConstBool alongside ConstInt in the IR (PatternInst and ExprInst), and (ii) codegen these properly as Rust bool types. We might also consider at some point making our "primitive" type hierarchy a bit richer and distinguishing ints and bools, but that's lower-priority.

github-actions[bot] commented 2 years ago

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "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)
fitzgen commented 2 years ago

FWIW, we use $true and $false elsewhere, eg:

https://github.com/bytecodealliance/wasmtime/blob/0580c84405cb5808518b187d1851dc17e7538108/cranelift/codegen/src/isa/x64/lower.isle#L24-L30

Not convinced that #t and #f is really that much of an improvement.

cfallin commented 2 years ago

Yep, we'd want to clean up said uses all at the same time.

I'm not as concerned about syntax; we could even just accept tokens true and false; my concern is more that $true and $false are abusing a feature meant for other things (externally-defined constants) and if we have bool values, we should represent them as such in the IR. Getting the type right there would also allow us to use them in the future in other DSL-compiler optimizations.