Skip to content

Conversation

@AlexMaclean
Copy link
Member

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.

@AlexMaclean AlexMaclean self-assigned this Nov 21, 2025
@github-actions
Copy link

🐧 Linux x64 Test Results

  • 186453 tests passed
  • 4868 tests skipped

//===----------------------------------------------------------------------===//

class ValueType<int size, int value> {
class ValueType<int size, int value = -1> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class ValueType<int size, int value = -1> {
class ValueType<int size, int value = ?> {

and use !initialized?

Copy link
Contributor

@s-barannikov s-barannikov left a 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?

@AlexMaclean
Copy link
Member Author

Can you confirm that the generated file is the same?

The file is largely the same. The only difference is that there used to be a large gap between c128 (the last normal value type) and cPTR (the first non-normal value type). This gap is no longer present after my change. I don't see any reason why we'd want this though.

def c64 : VTCheriCapability<64, 254>;   // 64-bit CHERI capability value
def c128 : VTCheriCapability<128, 255>; // 128-bit CHERI capability value
...
def cPTR : VTAny<503>;

Also, some targets use PtrValueType to define their own types; can this cause issues?

PtrValueType will still have the same value as the underlying scalar type it represents. These should be totally unaffected by this change.

Copy link
Contributor

@s-barannikov s-barannikov left a 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));
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Member Author

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.

@arsenm arsenm requested a review from chapuni November 24, 2025 23:27
@arsenm
Copy link
Contributor

arsenm commented Nov 24, 2025

I think @chapuni had a different patch for this at some point

@chapuni
Copy link
Contributor

chapuni commented Nov 25, 2025

@arsenm
I haven't had patches for this. I supposed we needed to introduce "autonumber" in tblgen.

I think this is a better workaround. Thanks! @AlexMaclean

@s-barannikov s-barannikov requested a review from jayfoad November 26, 2025 16:08
@s-barannikov
Copy link
Contributor

Alternative in #169670

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.

4 participants