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

[TableGen] Add a field to filter out GenericTable entries #65458

Merged

Conversation

wangpc-pp
Copy link
Contributor

A field FilterClassField is added to GenericTable class, which
is an optional bit field of FilterClass. If specified, only those
records with this field being true will have corresponding entries
in the table.

Copy link
Collaborator

@statham-arm statham-arm left a comment

Choose a reason for hiding this comment

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

The change looks OK to me, but this should surely have a test in llvm/test/TableGen.

You don't say what the use case for this is. I'm guessing it might be so you can construct two GenericTables with the same FilterClass, so that they select from the same overall set of records, but assign them different FilterClassField values so that they include different subsets of the records of that class?

Perhaps demonstrating that in the new test would be a good idea, and maybe mentioning it in the documentation too. (Documentation is at least as much for "what feature can I use to do this thing?" as "what does this feature do?")

@wangpc-pp
Copy link
Contributor Author

The change looks OK to me, but this should surely have a test in llvm/test/TableGen.

Thanks! I was planning to add a test but I can't find existed tests for GenericTable. I will try to add one later.

You don't say what the use case for this is. I'm guessing it might be so you can construct two GenericTables with the same FilterClass, so that they select from the same overall set of records, but assign them different FilterClassField values so that they include different subsets of the records of that class?

Yes! This can be a scenario! And I posted another PR #65460 to show the usage.

Perhaps demonstrating that in the new test would be a good idea, and maybe mentioning it in the documentation too. (Documentation is at least as much for "what feature can I use to do this thing?" as "what does this feature do?")\

Will do, thanks!

Copy link
Collaborator

@statham-arm statham-arm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

A field `FilterClassField` is added to `GenericTable` class, which
is an optional bit field of `FilterClass`. If specified, only those
records with this field being true will have corresponding entries
in the table.
Add a test in `llvm/test/TableGen/generic-tables.td` and make
detailed document
@wangpc-pp wangpc-pp force-pushed the main-tablegen-generic-table-filter-field branch from 70c41be to c1864aa Compare September 7, 2023 12:12
Copy link
Contributor

@michaelmaitland michaelmaitland left a comment

Choose a reason for hiding this comment

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

LGTM

llvm/docs/TableGen/BackEnds.rst Outdated Show resolved Hide resolved
@wangpc-pp wangpc-pp merged commit 2f78081 into llvm:main Sep 8, 2023
2 checks passed
@wangpc-pp wangpc-pp deleted the main-tablegen-generic-table-filter-field branch September 8, 2023 08:27
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.

None yet

3 participants