-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
ISLE: Add proper bool type #9593
Conversation
2db83b3
to
7d0993f
Compare
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:x64", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this -- it's nice to see motion on the "nice-to-have" upgrades we've had in our backlog for a while!
Mostly looks fine; my main thought is about the BuiltinType
abstraction and whether we can separate the change out.
@@ -80,9 +80,36 @@ pub struct TypeEnv { | |||
pub errors: Vec<Error>, | |||
} | |||
|
|||
/// A built-in type. | |||
#[derive(Copy, Clone, Debug, PartialEq, Eq)] | |||
pub enum BuiltinType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the reason for having a notion of "built-in types" for bool but not for other types that we have constants for (namely integers) -- while on the other hand it adds some complication to the IR. Maybe we could separate out this part into another PR and discuss separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(To make it more explicit: I think we probably should have this eventually; but for integers too, and then we should check that integer literals are used only in contexts that expect them.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention is to extend BuiltinType
to include other types (like u{8,16,32,64,size}
, i{8,16,32,64,size}
or str
) in future PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, yeah, on further thought I suppose it's totally fine to migrate integers over as a second step -- as long as the intention is there to finish out the refactor. Thanks!
Copyright (c) 2024, Arm Limited. Signed-off-by: Karl Meakin <karl.meakin@arm.com>
Copyright (c) 2024, Arm Limited. Signed-off-by: Karl Meakin <karl.meakin@arm.com>
Copyright (c) 2024, Arm Limited. Signed-off-by: Karl Meakin <karl.meakin@arm.com>
Copyright (c) 2024, Arm Limited. Replace all occurences of `$true` and `$false` with `true` and `false`. Signed-off-by: Karl Meakin <karl.meakin@arm.com>
Instead of threading `on_lhs` through all the calls to `translate_expr`, we can just set `is_partial` and `is_pure` on `root_flags` to true. Copyright (c) 2024, Arm Limited. Signed-off-by: Karl Meakin <karl.meakin@arm.com>
Copyright (c) 2024, Arm Limited. Signed-off-by: Karl Meakin <karl.meakin@arm.com>
7d0993f
to
e2f2d70
Compare
Partially solves #3573