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

astcenc: Update to 4.7.0 #80375

Merged
merged 1 commit into from
Mar 1, 2024
Merged

Conversation

akien-mga
Copy link
Member

The 4.5.0 release is a minor release with minor image quality improvements,
and multiple build system quality-of-life improvements.

https://github.com/ARM-software/astc-encoder/releases/tag/4.5.0

Draft as we need to review the compile flags for the "invariant" mode, which seems to be what we use (since we don't define ASTCENC_NO_INVARIANCE:

https://github.com/ARM-software/astc-encoder/blob/008d43f1e57944fd6c5cb663ef2c21c4884835dd/Source/cmake_core.cmake#L231-L244

See all the changelog lines talking about invariance. I'm assuming this is something we want to ensure if possible @fire? (IIRC, this means having reproducible encodings on different systems, at the cost of some performance.)

@akien-mga akien-mga requested a review from a team August 7, 2023 13:42
@akien-mga akien-mga added this to the 4.2 milestone Aug 7, 2023
@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 7, 2023
@fire
Copy link
Member

fire commented Aug 7, 2023

The cmake appears to be for msvc enable strict for older and precise for newer. There’s also a clang cl change.

For floating point math.

@akien-mga
Copy link
Member Author

akien-mga commented Aug 7, 2023

The cmake appears to be for msvc enable strict for older and precise for newer. There’s also a clang cl change.

For floating point math.

Yeah, and also for regular Clang and GCC:

                $<$<AND:${is_clang},$<VERSION_GREATER_EQUAL:$<CXX_COMPILER_VERSION>,10.0.0>>:-ffp-model=precise>
                $<${is_gnu_fe}:-ffp-contract=off>)

(is_gnu_fe is for GCC and Clang IIUC)

It's pretty fine grained, I'm not sure we want to reproduce all this exactly, but I'll see what can be done easily.

@fire
Copy link
Member

fire commented Aug 7, 2023

Since I don’t have guidance, we can try reproducing the code.

@fire
Copy link
Member

fire commented Aug 8, 2023

I slept on this.

This is trying to make the exported ASTC conversions stable across different machines with respect to math.

@akien-mga
Copy link
Member Author

I added the same flags as upstream, aside from the ones for ClangCL as we don't officially support it yet, and the tooling we have in methods.py doesn't let me detect it easily.

@akien-mga akien-mga marked this pull request as ready for review August 17, 2023 13:34
@akien-mga akien-mga requested review from a team as code owners August 17, 2023 13:34
@akien-mga akien-mga modified the milestones: 4.2, 4.3 Oct 30, 2023
@akien-mga akien-mga added cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release and removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Oct 30, 2023
@Chubercik
Copy link
Contributor

What's the state of this PR? There have been 3 releases since 4.5.0, currently sitting at 4.7.0.

SConstruct Outdated
cc_version_major = int(cc_version["major"] or -1)
cc_version_minor = int(cc_version["minor"] or -1)
cc_version_metadata1 = cc_version["metadata1"] or ""
env.compiler_version = (int(cc_version["major"]), int(cc_version["minor"]), int(cc_version["patch"]))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this refactor be in a separate PR from the astcenc library update? Perhaps as a prerequisite?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is :D #80719
But I'm not finding the time to polish it properly.

I might update this PR and just drop the compiler-specific tweaks, to add later.

@akien-mga akien-mga changed the title astcenc: Update to 4.5.0 astcenc: Update to 4.7.0 Feb 29, 2024
Comment on lines 46 to 52
if env.msvc:
# FIXME: MSVC compiler version detection isn't well implemented yet.
pass
# if env.compiler_version >= (19, 30, 0):
# env_thirdparty.AppendUnique(CCFLAGS=["/fp:precise"])
# else:
# env_thirdparty.AppendUnique(CCFLAGS=["/fp:strict"])
Copy link
Member Author

Choose a reason for hiding this comment

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

Will look into this later, but if someone can test on MSVC and confirm that astc encoding still seems to behave ok, I think this can be merged already.

@akien-mga
Copy link
Member Author

Removed all the compiler flag stuff for now, here's the old code for future reference:

# Compiler options to ensure invariant build.
# Sync with upstream `Source/cmake_core.cmake` options for ASTCENC_INVARIANCE.
if env.msvc:
    pass
    if env.compiler_version >= (19, 30, 0):
       env_thirdparty.AppendUnique(CCFLAGS=["/fp:precise"])
   else:
       env_thirdparty.AppendUnique(CCFLAGS=["/fp:strict"])
else:
    from methods import using_clang

    if using_clang(env) and env.compiler_version >= (10, 0, 0):
        env_thirdparty.AppendUnique(CCFLAGS=["-ffp-model=precise"])
    env_thirdparty.AppendUnique(CCFLAGS=["-ffp-contract=off"])

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

You only live once merge?

@akien-mga akien-mga merged commit 9538372 into godotengine:master Mar 1, 2024
16 checks passed
@akien-mga akien-mga deleted the astcenc-4.5.0 branch March 1, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release enhancement topic:thirdparty
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants