Bug 220896 - Roll ANGLE to include upstreamed Metal backend
Summary: Roll ANGLE to include upstreamed Metal backend
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kyle Piddington
URL:
Keywords: InRadar
: 230216 232905 233817 (view as bug list)
Depends on: 232064 228240 233279
Blocks: 232367 233007 233825 218949 anglemetal 233817 233837 233952 235278 235281 236427 236699 236733
  Show dependency treegraph
 
Reported: 2021-01-23 14:09 PST by Dean Jackson
Modified: 2022-02-17 15:12 PST (History)
27 users (show)

See Also:


Attachments
changes.diff from experimental rebase (377.94 KB, patch)
2021-11-08 17:24 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (41.41 MB, patch)
2021-11-30 13:55 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (41.41 MB, patch)
2021-11-30 17:37 PST, Kyle Piddington
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (41.43 MB, patch)
2021-12-01 16:54 PST, Kyle Piddington
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix WinCairo build (11.20 KB, patch)
2021-12-01 18:21 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (41.43 MB, patch)
2021-12-02 22:47 PST, Kyle Piddington
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (41.43 MB, patch)
2021-12-02 23:52 PST, Kyle Piddington
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (41.44 MB, patch)
2021-12-03 13:36 PST, Kyle Piddington
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (41.44 MB, patch)
2021-12-06 17:08 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (41.44 MB, patch)
2021-12-06 18:48 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2021-01-23 14:09:19 PST
Once https://chromium-review.googlesource.com/2618530 (upstreaming Apple's Metal backend to ANGLE) has landed, we'll take the new ANGLE into WebKit.
Comment 1 Radar WebKit Bug Importer 2021-01-23 14:09:26 PST
<rdar://problem/73539682>
Comment 2 Kenneth Russell 2021-07-23 17:38:58 PDT
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.
Comment 3 Kenneth Russell 2021-11-08 17:20:12 PST
Perhaps this should be blocked on Bug 232064 to indicate that we should ideally revise the update-angle script to handle this scenario.
Comment 4 Kyle Piddington 2021-11-08 17:24:03 PST
Created attachment 443636 [details]
changes.diff from experimental rebase
Comment 5 Kyle Piddington 2021-11-08 17:41:05 PST
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?
Comment 6 Kyle Piddington 2021-11-30 13:55:40 PST
Created attachment 445467 [details]
Patch
Comment 7 Kyle Piddington 2021-11-30 17:37:51 PST
Created attachment 445495 [details]
Patch
Comment 8 EWS Watchlist 2021-11-30 17:45:45 PST
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 9 Kyle Piddington 2021-12-01 16:54:43 PST
Created attachment 445635 [details]
Patch
Comment 10 Fujii Hironori 2021-12-01 18:21:29 PST
Created attachment 445652 [details]
Fix WinCairo build

Please merge this fix for your next patch.
Comment 11 Kyle Piddington 2021-12-02 22:47:43 PST
Created attachment 445812 [details]
Patch
Comment 12 Kyle Piddington 2021-12-02 23:52:02 PST
Created attachment 445817 [details]
Patch
Comment 13 Kyle Piddington 2021-12-03 13:36:28 PST
Created attachment 445892 [details]
Patch
Comment 14 Kyle Piddington 2021-12-06 17:08:03 PST
Created attachment 446103 [details]
Patch
Comment 15 Kyle Piddington 2021-12-06 18:48:36 PST
Created attachment 446107 [details]
Patch
Comment 16 Kenneth Russell 2021-12-06 22:45:43 PST
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.
Comment 17 Dean Jackson 2021-12-07 11:52:30 PST
Committed revision 286603.
Comment 18 Dean Jackson 2021-12-07 11:53:15 PST
https://trac.webkit.org/changeset/286603/webkit
Comment 19 Kenneth Russell 2021-12-07 13:08:49 PST
Fantastic work Kyle managing this difficult forward roll! All subsequent ones will be much easier, and looking forward to collaborating on them!
Comment 20 Yusuke Suzuki 2021-12-07 18:50:06 PST
Committed r286638 (244951@trunk): <https://commits.webkit.org/244951@trunk>
Comment 21 Kenneth Russell 2021-12-15 11:39:48 PST
*** Bug 233817 has been marked as a duplicate of this bug. ***
Comment 22 Kyle Piddington 2022-01-19 11:15:21 PST
*** Bug 232905 has been marked as a duplicate of this bug. ***
Comment 23 Kyle Piddington 2022-02-03 15:41:40 PST
*** Bug 230216 has been marked as a duplicate of this bug. ***
Comment 24 Darin Adler 2022-02-17 11:36:00 PST
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