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

[Support] Allow erasing names in Namespace #7424

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

mortbopet
Copy link
Contributor

@mortbopet mortbopet commented Aug 2, 2024

Allowing erasing names in a namespace seems more sane than micro-managing which names gets added to a namespace. E.g., it's convenient to use Namespace::add(SymbolCache & to efficiently prime a namespace, and then surgically removing some known identifier,, instead of having to re-implement how symbols are added to the namespace.

Required for #7425.

Allowing erasing names in a namespace seems more sane than micro-managing which names gets added to a namespace. E.g., it's convenient to use `Namespace::add(SymbolCache &` to efficiently prime a namespace, and then surgically removing some known identifier,, instead of having to re-implement how symbols are added to the namespace.
Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me!

@dtzSiFive
Copy link
Contributor

The namespace data-structure is not a simple set of reserved names, it's a map of prefixes to index, and erasing a single string can easily leave it in a bit of an inconsistent state (such that the removed name won't appear available consistently depending on whether it was directly requested or not).

As one example, given sym_2_2 it's possible there are other entries that are currently "remembering" that _2 has been taken.
The suffix path tracks this in a different way and in theory I believe could have multiple entries that need updating to really make the specified name available.

This is an optimization so that newName("foo") done 1000 times doesn't generate/check 1, 2, 3, 4... 1000 names and instead each only checks the base is taken and the next remembered not-tried index (or thereabouts, reading the code is probably best source for the specifics).

It seems undesirable to leave the data-structure in that state.

I haven't looked at the linked PR yet to grok the motivation so I'm not sure what alternatives to suggest. WDYT?

@mortbopet
Copy link
Contributor Author

@dtzSiFive this is obviously a gun that we're providing users to shoot themselves with, if they're using the erase function after they've started calling newName. The overall issue here is to figure out a way to opt-out of symbols being added to the namespace.

Currently, there isn't really a way to do that, since the only two add methods that Namespace provides are scanning an mlir::ModuleOp and inserting all operations or using a SymbolCache. One can manually build a symbol cache, but i personally think that's a bit dumb to do, seeing as one would basically have to re-implement symbol detection via. op walking, while adding a manual filter in there.

I don't really care how this is done, as long as it's possible. If it's more paletable to add an optional filter argument to this, i'd be supportive of that. Although that'll be a bit worse, performance related in the usecase of #7425.

We can also just add a locked flag to the namespace, which will be set after the first call to newName. After that, it's illegal to erase names from the namespace.

@mortbopet mortbopet force-pushed the dev/mpetersen/namespace_erase branch from a73c2cd to fabf2ed Compare August 6, 2024 11:05
@mortbopet
Copy link
Contributor Author

Since this is blocking internal work, i'm going to go ahead and merge. @dtzSiFive feel free to raise a post-merge issue if you think the solution is too controversial - I added a locked variable to the namespace, which gets set upon calling getName. Thus, any use of erase after a call to getName is illegal, and detected.

@mortbopet mortbopet merged commit fd2b37a into main Aug 6, 2024
4 checks passed
@mortbopet mortbopet deleted the dev/mpetersen/namespace_erase branch August 6, 2024 12:28
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.

3 participants