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

Initialize string cache in context #349

Merged
merged 23 commits into from
Oct 17, 2023
Merged

Initialize string cache in context #349

merged 23 commits into from
Oct 17, 2023

Conversation

raviqqe
Copy link
Member

@raviqqe raviqqe commented Oct 14, 2023

This PR removes the global static string cache that stores CString in MLIR (C++) for each corresponding &str in Rust. Instead, we have a cache in each context. And, that forces users to pass around the new Context type accordingly.

Close #54.

This change is also the base of #271, #23, and #348.

@raviqqe
Copy link
Member Author

raviqqe commented Oct 14, 2023

@edg-l @Danacus Can any of you review this PR and provide feedback if any?

@Danacus
Copy link
Contributor

Danacus commented Oct 14, 2023

Looks good to me! I'm just wondering if you can just use self.context() instead of passing the context as an argument to the methods of Operation? Otherwise it would be possible for users to pass a different Context to the operation, which might have unexpected consequences.

@raviqqe
Copy link
Member Author

raviqqe commented Oct 16, 2023

I'm just wondering if you can just use self.context() instead of passing the context as an argument to the methods of Operation?

I'm not sure if you are asking if it's possible to add a context field to Operation or if it's possible to use the existing Operation::context() method to fetch the context.

For the former, we can but then we cannot use the Operation values as raw values without explicitly casting. This is problematic when we want to convert an array of Operation values. For example, we won't be able to do &[operation1, operation2] as *MlirOperation.

For the latter, we cannot in general because we use mlirOperationGetContext() under the hood in the Operation::context() method.

Otherwise it would be possible for users to pass a different Context to the operation, which might have unexpected consequences.

I'm thinking of introducing dynamic checks for #271 instead. I think we need those checks anyway to check, for example, if given attributes are from the same context as existing attributes in an operation.

@Danacus
Copy link
Contributor

Danacus commented Oct 17, 2023

I meant the latter, but I didn't realize using mlirOperationGetContext cannot be used in general.

@edg-l
Copy link
Member

edg-l commented Oct 17, 2023

looks good to me!

@raviqqe
Copy link
Member Author

raviqqe commented Oct 17, 2023

Thank you everyone for reviews!

@raviqqe raviqqe merged commit c15cecf into main Oct 17, 2023
11 checks passed
@raviqqe raviqqe deleted the chore/enforced-context branch October 17, 2023 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove hacky StringRef cache
3 participants