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
Add signedness semantics for integer types #201
Conversation
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.
LGTM, module some changes that feel unrelated to me. I generally feel these maybe should be their own PR, as it confused me a bit in this PR.
} | ||
} | ||
func.func() ["sym_name" = "tuple_to_tuple", "value" = !fun<[!tuple<[!i32]>], [!tuple<[!i32]>]>, "function_type" = !fun<[], []>] {} | ||
// CHECK: "value" = !fun<[!tuple<[!i32]>], [!tuple<[!i32]>]> |
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 don't quite get this change. AFAIU, you are using a function operation here to show that !fun<[!tuple<[!i32]>], [!tuple<[!i32]>]>
can be parsed/printed. Why use a function operation? Aren't there easier operations to do that with? You can give sym_name
and value
to any operation to keep the name of the test around as well, right?
I feel like I am confused by these tests each time I see them. Would it make sense to have an empty test.testing
operation just to make these tests easier to understand?
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.
Yeah, I'll add a test.testing
in a following PR, that should make things better!
This PR adds signedness semantics for integer types, as MLIR does.
This PR also fix one test that was failing, but not running on the CI since the MLIR tests are not ran on the CI.
(Note, I should fix in a following PR this, since most of the MLIR files do not need MLIR anymore).