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

Restore support for older macOS/iOS version. Determine features at init, and use at runtime. #3284

Merged
merged 4 commits into from
May 3, 2024

Conversation

mcourteaux
Copy link
Contributor

A lot of the warnings could be silenced by doing what Apple suggests: using the @available check at the code. However, I agree that readability increases if you name the feature you are testing for, instead of just random-looking version number checks.

Examples still work.

@bkaradzic
Copy link
Owner

Instead of:

CHECK_FEATURE_AVAILABLE(m_usesNewMetalAPI, macOS 13.0, iOS 16.0, tvOS 16.0, macCatalyst 16.0, VISION_OS_MINIMUM *);

Why you don't do this?

m_usesNewMetalAPI = @available(macOS 13.0, iOS 16.0, tvOS 16.0, macCatalyst 16.0, VISION_OS_MINIMUM *);

And what's this macCatalyst?

@bkaradzic
Copy link
Owner

I just read that @available is executed only on startup, and it's not evaluated multiple times in runtime?

@bkaradzic
Copy link
Owner

Also you might want to switch visionOS to #available visionOS since #available is evaluated during compile time.

@mcourteaux
Copy link
Contributor Author

Also you might want to switch visionOS to #available visionOS since #available is evaluated during compile time.

I thought #available is a Swift feature.

I just read that @available is executed only on startup, and it's not evaluated multiple times in runtime?
Well, given that Apple so aggressively wants us to use it everywhere, that would make sense. I haven't read anything about it's performance or how it's implemented.

And what's this macCatalyst?

It seems to be some SDK variant that works for both macOS and iPad. Like where you can write your app once against the macCatalyst SDK and you get it on both.

Why you don't do this?

 m_usesNewMetalAPI = @available(macOS 13.0, iOS 16.0, tvOS 16.0, macCatalyst 16.0, VISION_OS_MINIMUM *);

Compiler complains that this is not the correct way to use @available. Literally the only valid way seems to be: if (@available(...)) { ... } with potentially an else branch. Even putting ! in front of the @available() is not valid. Compiler will warn that it's not actually going to guard against version checks. Some folks online said that this @available is not a boolean expression 🤷. Only valid to use in an if-condition. That would suggest they do some clever codegen regarding those branches. I wouldn't be surprised if it ends up being a single jmp [mem] instruction without any comparisons/tests. I may try to find what the compiler compiles here in the disassembly if I find some time. If performance is good or better, we could even do:

#define USES_NEW_API @available(macOS 13.0, iOS 16.0, tvOS 16.0, macCatalyst 16.0, VISION_OS_MINIMUM *)

if (USES_NEW_API) {

} else {

}

@bkaradzic
Copy link
Owner

I may try to find what the compiler compiles here in the disassembly if I find some time.

Yes, please do.

Because then this would be solution to have no warnings, and by looking at the code it would be clear what's going on:

#define USES_NEW_API @available(macOS 13.0, iOS 16.0, tvOS 16.0, macCatalyst 16.0, VISION_OS_MINIMUM *)

if (USES_NEW_API)
...

@bkaradzic
Copy link
Owner

I thought #available is a Swift feature.

From:
https://developer.apple.com/documentation/swift/marking-api-availability-in-objective-c

In Swift, you use the @available attribute to control whether a declaration is available to use when building an app for a particular target platform. Similarly, you use the availability condition #available to execute code conditionally based on required platform and version conditions. Both kinds of availability specifier are also available in Objective-C.

@bkaradzic
Copy link
Owner

Also usesNewMetalAPI is bad feature name... You can imagine Metal will add even newer API in the future.

The best feature naming is to be specific about feature you're naming.

@mcourteaux
Copy link
Contributor Author

mcourteaux commented May 1, 2024

Looking at a compiled version of a simple @available check, gives me this under -O2 (pseudocode generated by Hopper Disassembler):

int _main() {
    puts("before");
    COND = ___isPlatformVersionAtLeast(0x1, 0xd, 0x0, 0x0) == 0x0;
    rdi = "macOS 13.0";
    if (COND) {
            rdi = "NOT macOS 13.0";
    }
    puts(rdi);
    return 0x0;
}


int ___isPlatformVersionAtLeast(int arg0, int arg1, int arg2, int arg3) {
    stack[-16] = r15;
    stack[-24] = r14;
    stack[-32] = r12;
    stack[-40] = rbx;
    rsp = rsp - 0x30;
    r14 = arg3;
    r12 = arg2;
    rbx = arg1;
    rbp = arg0;
    if (*_DispatchOnceCounter == 0xffffffffffffffff) {
            if (*_AvailabilityVersionCheck != 0x0) {
                    rax = _AvailabilityVersionCheck(0x1, &var_38, r14 & 0xff | r12 << 0x8 & 0xffff | rbx << 0x10, r12 << 0x8 & 0xffff | rbx << 0x10);
                    r15 = rax;
            }
            else {
                    r15 = 0x1;
                    if (rbp == 0x1) {
                            if (*_CompatibilityDispatchOnceCounter == 0xffffffffffffffff) {
                                    r15 = 0x1;
                                    if (*(int32_t *)_GlobalMajor <= rbx) {
                                            if (*(int32_t *)_GlobalMajor >= rbx) {
                                                    r15 = 0x1;
                                                    if (*(int32_t *)_GlobalMinor <= r12) {
                                                            if (*(int32_t *)_GlobalMinor < r12) {
                                                                    r15 = 0x0;
                                                            }
                                                            else {
                                                                    r15 = *(int32_t *)_GlobalSubminor >= r14 ? 0x1 : 0x0;
                                                            }
                                                    }
                                            }
                                            else {
                                                    r15 = 0x0;
                                            }
                                    }
                            }
                            else {
                                    dispatch_once_f(_CompatibilityDispatchOnceCounter, 0x0, _compatibilityInitializeAvailabilityCheck);
                                    if (*(int32_t *)_GlobalMajor > rbx) {
                                            r15 = 0x1;
                                    }
                                    else {
                                            if (*(int32_t *)_GlobalMajor >= rbx) {
                                                    r15 = 0x1;
                                                    if (*(int32_t *)_GlobalMinor <= r12) {
                                                            if (*(int32_t *)_GlobalMinor < r12) {
                                                                    r15 = 0x0;
                                                            }
                                                            else {
                                                                    r15 = *(int32_t *)_GlobalSubminor >= r14 ? 0x1 : 0x0;
                                                            }
                                                    }
                                            }
                                            else {
                                                    r15 = 0x0;
                                            }
                                    }
                            }
                    }
            }
    }
    else {
            dispatch_once_f(_DispatchOnceCounter, 0x0, _initializeAvailabilityCheck);
            if (*_AvailabilityVersionCheck == 0x0) {
                    r15 = 0x1;
                    if (rbp == 0x1) {
                            if (*_CompatibilityDispatchOnceCounter == 0xffffffffffffffff) {
                                    r15 = 0x1;
                                    if (*(int32_t *)_GlobalMajor <= rbx) {
                                            if (*(int32_t *)_GlobalMajor >= rbx) {
                                                    r15 = 0x1;
                                                    if (*(int32_t *)_GlobalMinor <= r12) {
                                                            if (*(int32_t *)_GlobalMinor < r12) {
                                                                    r15 = 0x0;
                                                            }
                                                            else {
                                                                    r15 = *(int32_t *)_GlobalSubminor >= r14 ? 0x1 : 0x0;
                                                            }
                                                    }
                                            }
                                            else {
                                                    r15 = 0x0;
                                            }
                                    }
                            }
                            else {
                                    dispatch_once_f(_CompatibilityDispatchOnceCounter, 0x0, _compatibilityInitializeAvailabilityCheck);
                                    if (*(int32_t *)_GlobalMajor > rbx) {
                                            r15 = 0x1;
                                    }
                                    else {
                                            if (*(int32_t *)_GlobalMajor >= rbx) {
                                                    r15 = 0x1;
                                                    if (*(int32_t *)_GlobalMinor <= r12) {
                                                            if (*(int32_t *)_GlobalMinor < r12) {
                                                                    r15 = 0x0;
                                                            }
                                                            else {
                                                                    r15 = *(int32_t *)_GlobalSubminor >= r14 ? 0x1 : 0x0;
                                                            }
                                                    }
                                            }
                                            else {
                                                    r15 = 0x0;
                                            }
                                    }
                            }
                    }
            }
            else {
                    rax = _AvailabilityVersionCheck(0x1, &var_38, r14 & 0xff | r12 << 0x8 & 0xffff | rbx << 0x10, r12 << 0x8 & 0xffff | rbx << 0x10);
                    r15 = rax;
            }
    }
    if (**___stack_chk_guard == **___stack_chk_guard) {
            rax = r15 & 0xff;
    }
    else {
            rax = __stack_chk_fail();
    }
    return rax;
}

So it does get initialized once, but afterwards the check is still quite involved as it doesn't merge the major,minor,patch numbers into one big number, so a lot of branches are evaluated needlessly... 😞 Our approach should still be faster unfortunately. Code of the helper function does not change under different levels -O0 to -O3.

@bkaradzic
Copy link
Owner

Is there way to disable deprecated warnings with some define before including Apple's header files? Or we have to do it with pragma? Since you're checking features with @available, disabling deprecated warnings across whole file is option.

@mcourteaux
Copy link
Contributor Author

@bkaradzic Have a look at the latest version. I silenced the warnings with specific pragmas. I am kind of bummed that the whole list of texture formats is silenced by one block, as it is informative to have the compiler yell at us if the list has a format that is not supported. This makes it easy to spot unsupported formats in the future as well. Right now we'd miss out on that information.

Other than that, I don't know if you would accept that I put the #pragmas on the indent of the actual code. In general I personally like that better for readability if it really matters on the depth of that code, but maybe you're against it in general. I can modify that if you want.

@mcourteaux
Copy link
Contributor Author

From: https://developer.apple.com/documentation/swift/marking-api-availability-in-objective-c

In Swift, you use the @available attribute to control whether a declaration is available to use when building an app for a particular target platform. Similarly, you use the availability condition #available to execute code conditionally based on required platform and version conditions. Both kinds of availability specifier are also available in Objective-C.

I believe you misinterpreted that: I think what they mean is that in both Obj-C and Swift you can both specify and check for OS versions. The syntax is the languages is different, as documented below that.

Also usesNewMetalAPI is bad feature name... You can imagine Metal will add even newer API in the future.

The best feature naming is to be specific about feature you're naming.

I addressed this as well. It's now called m_usesMTLBindings.

src/renderer_mtl.mm Outdated Show resolved Hide resolved
@bkaradzic bkaradzic merged commit 1437b5c into bkaradzic:master May 3, 2024
11 checks passed
okwasniewski pushed a commit to okwasniewski/bgfx that referenced this pull request May 24, 2024
…it, and use at runtime. (bkaradzic#3284)

* Restore support for older macOS/iOS version. Determine features at init, and use at runtime.

* Fix typo for visionOS macro expansion.

* Silence warnings with pragmas and pointer casts where possible.

* Pragma macros.
@bwrsandman
Copy link
Contributor

Hey, just want to outline that VISION_OS_MINIMUM might have to be 15.2 as 15.0 and 15.1 may not have visionOS SDK.
See #3314

okwasniewski pushed a commit to okwasniewski/bgfx that referenced this pull request Jul 17, 2024
…it, and use at runtime. (bkaradzic#3284)

* Restore support for older macOS/iOS version. Determine features at init, and use at runtime.

* Fix typo for visionOS macro expansion.

* Silence warnings with pragmas and pointer casts where possible.

* Pragma macros.
okwasniewski pushed a commit to okwasniewski/bgfx that referenced this pull request Aug 1, 2024
…it, and use at runtime. (bkaradzic#3284)

* Restore support for older macOS/iOS version. Determine features at init, and use at runtime.

* Fix typo for visionOS macro expansion.

* Silence warnings with pragmas and pointer casts where possible.

* Pragma macros.
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.

3 participants