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.

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

ISLE: support bool types and constants properly #3573

Open
cfallin opened this issue Nov 30, 2021 · 3 comments
Open

ISLE: support bool types and constants properly #3573

cfallin opened this issue Nov 30, 2021 · 3 comments
Labels
enhancement isle Related to the ISLE domain-specific language

Comments

@cfallin
Copy link
Member

cfallin commented Nov 30, 2021

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.

@cfallin cfallin added the isle Related to the ISLE domain-specific language label Nov 30, 2021
@github-actions
Copy link

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.

@fitzgen
Copy link
Member

fitzgen commented Nov 30, 2021

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

(rule (lower (has_type (fits_in_64 ty)
(bconst $false)))
(value_reg (imm ty 0)))
(rule (lower (has_type (fits_in_64 ty)
(bconst $true)))
(value_reg (imm ty 1)))

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

@cfallin
Copy link
Member Author

cfallin commented Nov 30, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement isle Related to the ISLE domain-specific language
Projects
Status: No status
Development

No branches or pull requests

2 participants