-
Notifications
You must be signed in to change notification settings - Fork 314
Rewrite assembly to not need codegen #1786
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?
Rewrite assembly to not need codegen #1786
Conversation
|
CI Vulkan-Loader build queued with queue ID 556523. |
|
CI Vulkan-Loader build queued with queue ID 556540. |
|
CI Vulkan-Loader build # 3234 running. |
9fded67 to
d91334e
Compare
|
CI Vulkan-Loader build queued with queue ID 556560. |
|
CI Vulkan-Loader build # 3235 running. |
|
CI Vulkan-Loader build queued with queue ID 556578. |
|
CI Vulkan-Loader build # 3236 running. |
|
CI Vulkan-Loader build # 3236 failed. |
The original assembly relied on a build-time generated "gen_defines.asm" that contains various values and struct offsets needed by the assembly. This creates a lot of friction, as this build step must account for the numerous build environments and configurations that are possible, on top of needing an alternative path to handle cross-compilation. By modifying the assembly to no use extern variables, asm_offsets.c can contain all the necessary values as a compile-time value, such that at link time the values are available to the assembler. This does slightly increase runtime as we are replacing constants with variables, but the added benefits of simplifying the build step outweighs it. Doing the above required splitting the marmasm into separate files, one for 32 and the other for 64 bit. This is because there is no straight forward support for multiple architectures in a single file. Without codegen, determining which architecture to use was easiest by creating separate files.
d91334e to
3437c80
Compare
|
CI Vulkan-Loader build queued with queue ID 557058. |
|
CI Vulkan-Loader build queued with queue ID 557075. |
|
CI Vulkan-Loader build # 3238 running. |
|
CI Vulkan-Loader build # 3238 passed. |
|
This does seem to simplify the build-time/code generation quite a bit, but the runtime overhead of each trampoline is quite a bit higher with this approach from my reading. Have you done microbenchmarks to determine how expensive the overhead is in a worst case on some representative modern processors? If the cost ends up being too high, perhaps there's an easier way to parse the offsets out at build time? E.g., when I've needed similar things (Like a structure size) I've used things like readelf, which should be available for a target arch if gas is, to parse the symbol values out of object files directly at build time, rather than relying on python scripts to parse assembly. |
The original assembly relied on a build-time generated "gen_defines.asm" that contains various values and struct offsets needed by the assembly. This creates a lot of friction, as this build step must account for the numerous build environments and configurations that are possible, on top of needing an alternative path to handle cross-compilation. By modifying the assembly to no use extern variables, asm_offsets.c can contain all the necessary values as a compile-time value, such that at link time the values are available to the assembler. This does slightly increase runtime as we are replacing constants with variables, but the added benefits of simplifying the build step outweighs it.
Doing the above required splitting the marmasm into separate files, one for 32 and the other for 64 bit. This is because there is no straight forward support for multiple architectures in a single file. Without codegen, determining which architecture to use was easiest by creating separate files.