Bug 220896 – Roll ANGLE to include upstreamed Metal backend

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Dean Jackson]

Once https://chromium-review.googlesource.com/2618530 (upstreaming Apple's Metal backend to ANGLE) has landed, we'll take the new ANGLE into WebKit.

[Radar WebKit Bug Importer]

<rdar://problem/73539682>

[Kenneth Russell]

This turned out to be a larger task than originally expected and it's still underway. https://bugs.chromium.org/p/angleproject/issues/detail?id=5505 is the tracking ANGLE bug.

When this is done, it's likely we'll need to overwrite Apple's version of ANGLE with the upstreamed one rather than attempting to rebase all of the changes forward, because there will be many collisions that have already been resolved upstream.

The change to thread-local storage from Bug 228240 needs to be carefully preserved during this process. More care may be necessary in other areas as well.

[Kenneth Russell]

Perhaps this should be blocked on Bug 232064 to indicate that we should ideally revise the update-angle script to handle this scenario.

[Kyle Piddington]

Created attachment 443636 [details]
changes.diff from experimental rebase

[Kyle Piddington]

Comment on attachment 443636 [details]
changes.diff from experimental rebase

View in context: https://bugs.webkit.org/attachment.cgi?id=443636&action=review

Publishing first comments draft (Up to State.cpp)

> include/platform/FeaturesMtl.h:30
> +    Feature hasExplicitMemBarrier = {"has_explicit_mem_barrier", FeatureCategory::MetalFeatures,

Can Drop and unify this change.

> include/platform/FeaturesMtl.h:43
> +    Feature hasStencilOutput = {"has_stencil_output", FeatureCategory::MetalFeatures,

Can drop and unify this change.

> include/platform/PlatformMethods.h:19
> +#    if defined(__GNUC__) || defined(__clang__)

Drop and unify

> src/common/angleutils.h:-224
> -size_t FormatStringIntoVector(const char *fmt, va_list vararg, std::vector<char> &buffer);

In source angle repo, Keep

> src/common/utilities.cpp:9
> +// Older clang versions have a false positive on this warning here.

Possibly necessary?

> src/common/utilities.cpp:488
> +/**

Drop, rewritten in Upstream

> src/common/utilities.h:40
> +int VariableAttributeCount(GLenum type);

Drop, unneeded

> src/compiler/preprocessor/preprocessor_tab_autogen.cpp:3
> +/* Apple Note: For the avoidance of doubt, Apple elects to distribute this file under the terms of the BSD license. */

Very much keep

> src/compiler/translator/BaseTypes.h:1507
> +    case EvqSampleMask:             return "SampleMask";

Drop, doubled up below.

> src/compiler/translator/CodeGen.cpp:26
> +#ifdef ANGLE_ENABLE_METAL_SPIRV

Keep, needed for compiling without SpirV

> src/compiler/translator/Common.h:217
> +    constexpr const char *formatStr = std::numeric_limits<T>::is_signed ? "%d" : "%u";

Possibly back port?

> src/compiler/translator/IntermNode.cpp:438
> +TIntermBlock::TIntermBlock(std::initializer_list<TIntermNode *> stmts)

Drop, not needed in current transpiler

> src/compiler/translator/IntermNode.cpp:539
> +    auto &seq = *getSequence();

Return to upstream.

> src/compiler/translator/IntermNode.cpp:550
> +    auto &seq = *getSequence();

Return to upstream

> src/compiler/translator/IntermNode.cpp:-556
> -    if (position > getSequence()->size())

Return to upstream, just refactoring.

> src/compiler/translator/TranslatorMetal.cpp:46
> +constexpr ImmutableString kDiscardWrapperFuncName  = ImmutableString("ANGLEDiscardWrapper");

Return to upstream, unused translator

> src/compiler/translator/TranslatorMetal.cpp:77
> +ANGLE_NO_DISCARD bool EmulateInstanceID(TCompiler *compiler,

All hardware supports instancing, drop.

> src/compiler/translator/TranslatorMetal.cpp:168
> +        if (mEmulatedInstanceID)

All hardware supports instancing

> src/compiler/translator/TranslatorMetal.cpp:-288
> -    sink << "layout (constant_id=0) const bool " << mtl::kRasterizerDiscardEnabledConstName;

Use namespace

> src/compiler/translator/TranslatorMetal.cpp:367
> +// If the MSL fragment shader is non-void, we need to ensure

Probably drop, no longer in main repo

> src/compiler/translator/TranslatorMetal.h:47
> +    void enableEmulatedInstanceID(bool e) { mEmulatedInstanceID = e; }

Drop instanced ID

> src/compiler/translator/TranslatorMetalDirect/ConstantNames.h:1
> +//

Refactor: Either back port or drop.

> src/compiler/translator/TranslatorMetalDirect/Debug.h:1
> +//

Remove

> src/compiler/translator/TranslatorMetalDirect/EnvironmentVariable.h:1
> +//

Remove

> src/compiler/translator/TranslatorMetalUtils.cpp:1
> +//

Keep

> src/compiler/translator/TranslatorMetalUtils.h:1
> +//

Keep

> src/compiler/translator/ValidateAST.cpp:-167
> -            if (mParent.find(child) != mParent.end())

Revert

> src/compiler/translator/glslang_tab_autogen.cpp:3
> +/* Apple Note: For the avoidance of doubt, Apple elects to distribute this file under the terms of the BSD license. */

Keep

> src/compiler/translator/glslang_tab_autogen.h:3
> +/* Apple Note: For the avoidance of doubt, Apple elects to distribute this file under the terms of the BSD license. */

keep

> src/compiler/translator/tree_ops/PruneEmptyCases.cpp:20
> +bool AreEmptyBlocks(const TIntermSequence *statements);

Can drop the const to unify

> src/compiler/translator/tree_ops/d3d/RemoveSwitchFallThrough.cpp:47
> +    void outputSequence(const TIntermSequence *sequence, size_t startIndex);

Drop const to unify

> src/compiler/translator/util.cpp:800
> +bool IsOutputMetalDirect(ShShaderOutput output)

Drop

> src/gpu_info_util/SystemInfo_apple.mm:-2
> -// Copyright 2020 The ANGLE Project Authors. All rights reserved.

Use ANGLE Project Authors

> src/gpu_info_util/SystemInfo_macos.mm:362
> +#if defined(ANGLE_PLATFORM_MACCATALYST) && defined(ANGLE_CPU_ARM64)

Keep, from upstream

> src/libANGLE/Context.cpp:9675
> +

Just whitespace

> src/libANGLE/Display.cpp:1449
> +Error Display::makeCurrent(const Thread *thread,

Drop, replaced by new function

> src/libANGLE/Display.h:182
> +    Error makeCurrent(const Thread *thread,

Drop, replaced by new function

> src/libANGLE/Program.h:297
> +    void setLinkedTransformFeedbackVaryings(

Drop, in Program.h now.

> src/libANGLE/ProgramExecutable.h:225
> +    void setLinkedTransformFeedbackVaryings(

Remove, in Program.h now.

> src/libANGLE/State.cpp:9
> +// Older clang versions have a false positive on this warning here.

From previous changes.diff. New clang may be ok now?

[Kyle Piddington]

Created attachment 445467 [details]
Patch

[Kyle Piddington]

Created attachment 445495 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Kyle Piddington]

Created attachment 445635 [details]
Patch

[Fujii Hironori]

Created attachment 445652 [details]
Fix WinCairo build

Please merge this fix for your next patch.

[Kyle Piddington]

Created attachment 445812 [details]
Patch

[Kyle Piddington]

Created attachment 445817 [details]
Patch

[Kyle Piddington]

Created attachment 445892 [details]
Patch

[Kyle Piddington]

Created attachment 446103 [details]
Patch

[Kyle Piddington]

Created attachment 446107 [details]
Patch

[Kenneth Russell]

The code review tool doesn't work for a patch this large.

I've applied an earlier iteration of this patch locally and examined the changes.diff from upstream; it looks good. The vast majority of WebKit's changes have been upstreamed, and over a year's worth of divergent development has been resolved.

Great work Kyle getting this to this point. Please consider this r+ by me.

The WinCairo failure here is an infrastructure problem where a new file wasn't properly cleaned by the bot.

[Dean Jackson]

Committed revision 286603.

[Dean Jackson]

https://trac.webkit.org/changeset/286603/webkit

[Kenneth Russell]

Fantastic work Kyle managing this difficult forward roll! All subsequent ones will be much easier, and looking forward to collaborating on them!

[Yusuke Suzuki]

Committed r286638 (244951@trunk): <https://commits.webkit.org/244951@trunk>

[Kenneth Russell]

*** Bug 233817 has been marked as a duplicate of this bug. ***

[Kyle Piddington]

*** Bug 232905 has been marked as a duplicate of this bug. ***

[Kyle Piddington]

*** Bug 230216 has been marked as a duplicate of this bug. ***

[Darin Adler]

Turns out it was pretty risky to diverge like this and then roll in so many changes. It seems we don’t have enough regression test coverage. Since the roll, WebKit testing has uncovered these three regressions:

Bug 236427
Bug 236733
Bug 236699