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

vulkan system dep: determine version on cross builds #13910

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sp1ritCS
Copy link
Contributor

Currently, the vulkan system dep detects its vulkan version by building and running:

#include <stdio.h>
#include <vulkan/vulkan.h>

int main() {
    printf("%i.%i.%i", VK_VERSION_MAJOR(VK_HEADER_VERSION_COMPLETE),
                       VK_VERSION_MINOR(VK_HEADER_VERSION_COMPLETE),
                       VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE));
    return 0;
}

this causes cross builds that do not have the possibility of running on the build machine to evaluate the vulkan dependency with an 'Unknown' version.

The new implementation provides a fallback mechanism that first evaluates the same code as above (just without the stdio import) using the C Preprocessor for the host machine and then takes the evaluated main function, adds the required stdio and stdint headers and builds it using the build machine c compiler in order to get the vulkan version for the host machine.

@xclaesse
Copy link
Member

xclaesse commented Nov 18, 2024

That's a clever trick. The way it's traditionally done is using cc.compute_int():

major = cc.compute_int('VK_VERSION_MAJOR(VK_HEADER_VERSION_COMPLETE)', prefix: '#include <vulkan/vulkan.h>')
minor = cc.compute_int('VK_VERSION_MINOR(VK_HEADER_VERSION_COMPLETE)', prefix: '#include <vulkan/vulkan.h>')
patch = cc.compute_int('VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE)', prefix: '#include <vulkan/vulkan.h>')

But that's massively slower because it's implemented as a binary search of the value, compiling a program that fails to build until we guessed the right value. See https://github.com/mesonbuild/meson/blob/master/mesonbuild/compilers/mixins/clike.py#L470.

Now I'm wondering if cc.compute_int() could be reimplemented using this preprocessor trick, at least as the fastpath when possible.

@sp1ritCS
Copy link
Contributor Author

The way it's traditionally done is using cc.compute_int():

Oh I didn't see that

But that's massively slower because it's implemented as a binary search of the value, compiling a program that fails to build until we guessed the right value. See https://github.com/mesonbuild/meson/blob/master/mesonbuild/compilers/mixins/clike.py#L470.

God, that is truly horrifying 🤣

at least as the fastpath when possible

Besides the issue that a header included during preprocessing could theoretically contain @MESON_CODE_START@ and break the whole thing, this could sometimes lead to incorrect results. For example if I wanted to evaluate __builtin_ctz(0) for some AVR chip, the preprocessing pass will leave __builtin_ctz(0) untouched causing it to be passed to the build compiler which will likely evaluate it as 32, while the gcc-avr should evaluate it to 16 instead (on the other hand, __builtin_ctz(0) is undefined anyway so people shouldn't rely on it 🤔 ).

@eli-schwartz
Copy link
Member

I don't think we can or should rely on preprocess + native compile hacks for answering questions of any sort unless we know via some mechanism that the answer is required to be correct.

This could mean for example reading the vulkan headers and asserting that it always spits out non-platform-specific preprocessed text that can then be compiled by any platform to produce a correct answer without linking to the vulkan libraries.

Evaluating the size of an int isn't something you can guarantee is required to be correct, because if it were required to be correct then people wouldn't need to check it... using this kind of hack might be useful as a guess which can then be verified via the existing cross-safe logic, though, in order to cut down on the search space in optimal cases. But I don't think it's useful to even theorize about removing the final check.

@nirbheek
Copy link
Member

nirbheek commented Nov 19, 2024

Very confused by Eli's comment, so I'm just going to ignore that.

First off, this should use VK_API_VERSION_* instead of VK_VERSION_*. The latter is deprecated.

Second, I agree with @xclaesse that this check should just use compute_int(), which does work:

cc.compute_int('VK_API_VERSION_MAJOR(VK_HEADER_VERSION_COMPLETE)', prefix: '#include <vulkan/vulkan.h>', low: 0, guess: 1)
cc.compute_int('VK_API_VERSION_MINOR(VK_HEADER_VERSION_COMPLETE)', prefix: '#include <vulkan/vulkan.h>', low: 0, guess: 3)
cc.compute_int('VK_API_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE)', prefix: '#include <vulkan/vulkan.h>', low: 0)

Third, you've invented a very useful and interesting mechanism of detecting the computed value of defines in a header without running something. get_define() will just get the pre-processed value, which is insufficient in this case. The major version pre-processes to ((uint32_t)(((((uint32_t)(0)) << 29U) | (((uint32_t)(1)) << 22U) | (((uint32_t)(3)) << 12U) | ((uint32_t)(296)))) >> 22U).

I think we should use it to speed-up compute_int() in a separate commit, and I think we can check for the theoretical case you mentioned by asserting that find and rfind for @MESON_CODE_START@ point to the same location. I expect it will never happen in practice.

I don't think we should worry about supporting __builtin() for this feature, it's for computing expressions, not evaluating functions. If it works, that's just an accident.

@nirbheek
Copy link
Member

First off, this should use VK_API_VERSION_* instead of VK_VERSION_*. The latter is deprecated.

Actually, I guess we shouldn't do this, because it might break detecting older Vulkan SDKs. Maybe just leave a comment to that effect, in case they remove the macro in the future.

sp1ritCS added a commit to sp1ritCS/meson that referenced this pull request Nov 19, 2024
Currently, the vulkan system dep detects its vulkan version by building
and running:

int main() {
    printf("%i.%i.%i", VK_VERSION_MAJOR(VK_HEADER_VERSION_COMPLETE),
                       VK_VERSION_MINOR(VK_HEADER_VERSION_COMPLETE),
                       VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE));
    return 0;
}

this causes cross builds that do not have the possibility of running on
the build machine to evaluate the vulkan dependency with an 'Unknown'
version.

Instead of evaluating beforementioned piece of C code, the new
implementation will instead use cc.compute_int to evaluate the three
preprocessor macros.
This is relativly expensive for cross builds right now but further
optimizations can be made. See mesonbuild#13910 for more details.
sp1ritCS added a commit to sp1ritCS/meson that referenced this pull request Nov 19, 2024
Currently, the vulkan system dep detects its vulkan version by building
and running:

int main() {
    printf("%i.%i.%i", VK_VERSION_MAJOR(VK_HEADER_VERSION_COMPLETE),
                       VK_VERSION_MINOR(VK_HEADER_VERSION_COMPLETE),
                       VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE));
    return 0;
}

this causes cross builds that do not have the possibility of running on
the build machine to evaluate the vulkan dependency with an 'Unknown'
version.

Instead of evaluating beforementioned piece of C code, the new
implementation will instead use cc.compute_int to evaluate the three
preprocessor macros.
This is relativly expensive for cross builds right now but further
optimizations can be made. See mesonbuild#13910 for more details.
sp1ritCS added a commit to sp1ritCS/meson that referenced this pull request Nov 19, 2024
Currently, the vulkan system dep detects its vulkan version by building
and running:

int main() {
    printf("%i.%i.%i", VK_VERSION_MAJOR(VK_HEADER_VERSION_COMPLETE),
                       VK_VERSION_MINOR(VK_HEADER_VERSION_COMPLETE),
                       VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE));
    return 0;
}

this causes cross builds that do not have the possibility of running on
the build machine to evaluate the vulkan dependency with an 'Unknown'
version.

Instead of evaluating beforementioned piece of C code, the new
implementation will instead use cc.compute_int to evaluate the three
preprocessor macros.
This is relativly expensive for cross builds right now but further
optimizations can be made. See mesonbuild#13910 for more details.
sp1ritCS added a commit to sp1ritCS/meson that referenced this pull request Nov 19, 2024
Currently, the vulkan system dep detects its vulkan version by building
and running:

int main() {
    printf("%i.%i.%i", VK_VERSION_MAJOR(VK_HEADER_VERSION_COMPLETE),
                       VK_VERSION_MINOR(VK_HEADER_VERSION_COMPLETE),
                       VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE));
    return 0;
}

this causes cross builds that do not have the possibility of running on
the build machine to evaluate the vulkan dependency with an 'Unknown'
version.

Instead of evaluating beforementioned piece of C code, the new
implementation will instead use cc.compute_int to evaluate the three
preprocessor macros.
This is relativly expensive for cross builds right now but further
optimizations can be made. See mesonbuild#13910 for more details.
Currently, the vulkan system dep detects its vulkan version by building
and running:

int main() {
    printf("%i.%i.%i", VK_VERSION_MAJOR(VK_HEADER_VERSION_COMPLETE),
                       VK_VERSION_MINOR(VK_HEADER_VERSION_COMPLETE),
                       VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE));
    return 0;
}

this causes cross builds that do not have the possibility of running on
the build machine to evaluate the vulkan dependency with an 'Unknown'
version.

Instead of evaluating beforementioned piece of C code, the new
implementation will instead use cc.compute_int to evaluate the three
preprocessor macros.
This is relativly expensive for cross builds right now but further
optimizations can be made. See mesonbuild#13910 for more details.
@sp1ritCS
Copy link
Contributor Author

@nirbheek: I've tried using get_define in order to speed up cross_compute_int: sp1ritCS@ef74545

There are two issues with it: what headers should be included for the build machine? For example vulkan needs stdint.h as its expanded macros contain expanded stuff like uint32 and get_define produces the following code:

#include <vulkan/vulkan.h>
        #ifndef VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE)
        # define VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) "MESON_GET_DEFINE_UNDEFINED_SENTINEL"
        #endif
        "MESON_GET_DEFINE_DELIMITER_START"
VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE)
"MESON_GET_DEFINE_DELIMITER_END"

which gcc doesn't really like and other compilers might fail 🤔

stderr:
/tmp/tmpxf0sitrq/testfile.c:3:33: warning: extra tokens at end of #ifndef directive [-Wextra-tokens]
    3 |         #ifndef VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE)
      |                                 ^
      |                                 //
1 warning generated.

@nirbheek
Copy link
Member

I think the compute_int() iteration will be really quick with the low and guess params, even if you don't use get_define(). How many iterations does it take right now?

@sp1ritCS
Copy link
Contributor Author

With guess being correct it obv. only takes one "iteration". The finding of the correct patch version takes 20 iterations.

@xclaesse
Copy link
Member

xclaesse commented Nov 19, 2024

With no guess or limits, 27 compilations:

compile VK_VERSION_MAJOR(VK_HEADER_VERSION_COMPLETE) >= 0
compile VK_VERSION_MAJOR(VK_HEADER_VERSION_COMPLETE) > 0
compile VK_VERSION_MAJOR(VK_HEADER_VERSION_COMPLETE) > 1
Computing int of "VK_VERSION_MAJOR(VK_HEADER_VERSION_COMPLETE)" : 1
compile VK_VERSION_MINOR(VK_HEADER_VERSION_COMPLETE) >= 0
compile VK_VERSION_MINOR(VK_HEADER_VERSION_COMPLETE) > 0
compile VK_VERSION_MINOR(VK_HEADER_VERSION_COMPLETE) > 1
compile VK_VERSION_MINOR(VK_HEADER_VERSION_COMPLETE) > 3
compile VK_VERSION_MINOR(VK_HEADER_VERSION_COMPLETE) <= 2
Computing int of "VK_VERSION_MINOR(VK_HEADER_VERSION_COMPLETE)" : 3
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) >= 0
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) > 0
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) > 1
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) > 3
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) > 7
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) > 15
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) > 31
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) > 63
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) > 127
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) > 255
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) > 511
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) <= 383
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) <= 319
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) <= 287
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) <= 271
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) <= 279
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) <= 275
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) <= 273
compile VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE) <= 274
Computing int of "VK_VERSION_PATCH(VK_HEADER_VERSION_COMPLETE)" : 275

That's actually not that bad, on my laptop that's instantaneous. Of course that kind of stuff adds up in the configure process. One thing meson can do is parallelize those a bit, I had a branch a while ago for that.

@nirbheek
Copy link
Member

Yeah, glib uses it for computing POLLIN etc, and I went to measure it and it was basically instant. I would recommend making this work first so you are unblocked on your gtk-android work, and we can also backport it easily to 1.6.x.

Then we can do the optimizations in a separate PR, which we won't backport.

@sp1ritCS
Copy link
Contributor Author

sp1ritCS commented Nov 19, 2024

Then we can do the optimizations in a separate PR, which we won't backport.

thats what I had in mind. This PR is basically ready (I don't think I'm responsible for breaking the windows tests) and my speedup_cross_compute_int branch is independent on this. I just didn't want to create a PR for it yet given the issues I've mentioned.

so you are unblocked on your gtk-android work

I'm fine with simply providing a vulkan.pc with gtk-android-builder, this doesn't really block much. #13800 is the far larger blocker 🙃

@nirbheek
Copy link
Member

Please open the PR anyway and we can keep the discussion in the right place :)

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