afonso360 edited issue #4766:
:wave: Hey,
Feature
This is a follow up to https://github.com/bytecodealliance/wasmtime/pull/4667#issuecomment-1218328143, where we discussed removing TestName as an option for Function names.
Benefit
The biggest benefit is cleaning up our naming interface. TestNames arguably shouldn't belong to the core cranelift infrastructure. Instead the test suite should have an external table as any other embedder would do.
The other benefit was allowing larger names in test functions, but @jameysharp has fixed that in https://github.com/bytecodealliance/wasmtime/pull/4764 (Thanks!)
@jameysharp tested this and while it does have some performance benefits, they are minimal.
This also removes some edges such as
cranelift-jit
not supporting test names which makes for a better experience using it.Implementation
I started working on this a few days ago, but I'm putting it on hold to fix some other more prioritary issues. (in progress branch) If anyone wants to pick this up in the mean time feel free!
The branch above starts by changing the parser to produce a side table of test names while assigning UserExernalNames to each function.
Following from that we need to switch the testsuite to use this new side table.
And after that we should be able to remove the TestName from cranelift.
This sounds simple, but I think it will end up being quite a big change.
Alternatives
Do nothing. This has been the state of cranelift for a long time. I think this would also break the interface for pretty much every downstream user since we would probably remove UserFuncName and replace it with UserExernalName directly.
afonso360 edited issue #4766:
:wave: Hey,
Feature
This is a follow up to https://github.com/bytecodealliance/wasmtime/pull/4667#issuecomment-1218328143, where we discussed removing TestName as an option for Function names.
Benefit
The biggest benefit is cleaning up our naming interface. TestNames arguably shouldn't belong to the core cranelift infrastructure. Instead the test suite should have an external table as any other embedder would do.
The other benefit was allowing larger names in test functions, but @jameysharp has fixed that in https://github.com/bytecodealliance/wasmtime/pull/4764 (Thanks!)
@jameysharp tested this and while it does have some performance benefits, they are minimal.
This also removes some edges such as
cranelift-jit
not supporting test names which makes for a better experience using it.Implementation
I started working on this a few days ago, but I'm putting it on hold to fix some other more prioritary issues. (in progress branch) If anyone wants to pick this up in the mean time feel free!
The branch above starts by changing the parser to produce a side table of test names while assigning UserExernalNames to each function.
Following from that we need to switch the testsuite to use this new side table.
And after that we should be able to remove the TestName from cranelift.
This sounds simple, but I think it will end up being quite a big change.
Alternatives
Do nothing. This has been the state of cranelift for a long time.
I think this would also break the interface for pretty much every downstream user since we would probably remove UserFuncName and replace it with UserExernalName directly.
bjorn3 commented on issue #4766:
The branch above starts by changing the parser to produce a side table of test names while assigning UserExernalNames to each function.
Would it be possible to provide this side table while printing functions?
afonso360 edited issue #4766:
:wave: Hey,
Feature
This is a follow up to https://github.com/bytecodealliance/wasmtime/pull/4667#issuecomment-1218328143, where we discussed removing TestName as an option for Function names.
Benefit
The biggest benefit is cleaning up our naming interface. TestNames arguably shouldn't belong to the core cranelift infrastructure. Instead the test suite should have an external table as any other embedder would do.
The other benefit was allowing larger names in test functions, but @jameysharp has fixed that in https://github.com/bytecodealliance/wasmtime/pull/4764 (Thanks!)
@jameysharp tested this and while it does have some performance benefits, they are minimal.
This also removes some edges such as
cranelift-jit
not supporting test names which makes for a better experience using it.Implementation
I started working on this a few days ago, but I'm putting it on hold to fix some other more prioritary issues. (in progress branch) If anyone wants to pick this up in the mean time feel free!
The branch above starts by changing the parser to produce a side table of test names while assigning UserExernalNames to each function.
Following from that we need to switch the testsuite to use this new side table.
And after that we should be able to remove the TestName from cranelift.
This sounds simple, but I think it will end up being quite a big change.
Alternatives
Do nothing. This has been the state of cranelift for a long time.
I think this would also break the interface for pretty much every downstream user since we would probably remove UserFuncName and replace it with UserExernalName directly.
Edit: Another alternative would be to expand this, and start supporting string function names, which would also be an interesting option.
afonso360 commented on issue #4766:
I think it should be possible, inside the test suite we use
TestCase
's so when printing those we can just query the name table.
clif-util run
i think also shows functions to the user when compilation fails, but I think it also uses theTestCase
(not quite sure).I don't know exactly where else we show these names to the user.
afonso360 edited a comment on issue #4766:
I think it should be possible, inside the test suite we use
TestCase
's so when printing those we can just query the name table.
clif-util run/compile
I think also shows functions to the user when compilation fails, but I think it also uses theTestCase
(not quite sure).I don't know exactly where else we show these names to the user.
bjorn3 commented on issue #4766:
We show them as part of
fn0 = %foo() -> i32, f32
function declarations inside the function body.
akirilov-arm labeled issue #4766:
:wave: Hey,
Feature
This is a follow up to https://github.com/bytecodealliance/wasmtime/pull/4667#issuecomment-1218328143, where we discussed removing TestName as an option for Function names.
Benefit
The biggest benefit is cleaning up our naming interface. TestNames arguably shouldn't belong to the core cranelift infrastructure. Instead the test suite should have an external table as any other embedder would do.
The other benefit was allowing larger names in test functions, but @jameysharp has fixed that in https://github.com/bytecodealliance/wasmtime/pull/4764 (Thanks!)
@jameysharp tested this and while it does have some performance benefits, they are minimal.
This also removes some edges such as
cranelift-jit
not supporting test names which makes for a better experience using it.Implementation
I started working on this a few days ago, but I'm putting it on hold to fix some other more prioritary issues. (in progress branch) If anyone wants to pick this up in the mean time feel free!
The branch above starts by changing the parser to produce a side table of test names while assigning UserExernalNames to each function.
Following from that we need to switch the testsuite to use this new side table.
And after that we should be able to remove the TestName from cranelift.
This sounds simple, but I think it will end up being quite a big change.
Alternatives
Do nothing. This has been the state of cranelift for a long time.
I think this would also break the interface for pretty much every downstream user since we would probably remove UserFuncName and replace it with UserExernalName directly.
Edit: Another alternative would be to expand this, and start supporting string function names, which would also be an interesting option.
Last updated: Dec 23 2024 at 12:05 UTC