-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[ValueTypes] Automatically assign value numbers (NFC) #169071
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
base: main
Are you sure you want to change the base?
Conversation
🐧 Linux x64 Test Results
|
| //===----------------------------------------------------------------------===// | ||
|
|
||
| class ValueType<int size, int value> { | ||
| class ValueType<int size, int value = -1> { |
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.
| class ValueType<int size, int value = -1> { | |
| class ValueType<int size, int value = ?> { |
and use !initialized?
s-barannikov
left a comment
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.
Can you confirm that the generated file is the same?
Also, some targets use PtrValueType to define their own types; can this cause issues?
The file is largely the same. The only difference is that there used to be a large gap between
PtrValueType will still have the same value as the underlying scalar type it represents. These should be totally unaffected by this change. |
s-barannikov
left a comment
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 modulo nit, thanks
| int Size = size; | ||
| int Value = value; | ||
| int Value = !if(!ne(value, -1), value, | ||
| !add(!size(!instances<ValueType>()), 1)); |
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.
This looks algorithmically terrible. Did you benchmark how long tablegen takes to generate this with and without this patch?
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.
plus it may influence all of the other backends that include ValueTypes.td
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.
Yep. This is pretty inefficient, but there doesn't seem to be a better way to implement this in table-gen.
I tested like this:
git checkout main
echo "main"
time ../cmake/bin/llvm-tblgen llvm/include/llvm/CodeGen/ValueTypes.td &> /dev/null
git checkout dev/amaclean/td-vts-indices
echo "dev/amaclean/td-vts-indices"
time ../cmake/bin/llvm-tblgen llvm/include/llvm/CodeGen/ValueTypes.td &> /dev/null
Here are the results:
Switched to branch 'main'
Your branch is up to date with 'origin/main'.
main
real 0m0.021s
user 0m0.018s
sys 0m0.002s
Switched to branch 'dev/amaclean/td-vts-indices'
dev/amaclean/td-vts-indices
real 0m0.094s
user 0m0.092s
sys 0m0.001s
So, when isolating ValueTypes.td, it makes things nearly 5X slower (which is pretty bad) but we're still only talking about less then 1/10th of a second which seems like it will be pretty negligible on the scale of a complete LLVM build.
|
I think @chapuni had a different patch for this at some point |
|
@arsenm I think this is a better workaround. Thanks! @AlexMaclean |
|
Alternative in #169670 |
The requirement to specify an index in each ValueType declaration adds a lot of visual clutter to the file and causes the addition of new value types to create somewhat painful chrun, especially if they are in a downstream branch which can create lots of merge conflicts.