Bug 220896

Summary: Roll ANGLE to include upstreamed Metal backend
Product: WebKit Reporter: Dean Jackson <dino>
Component: WebGLAssignee: Kyle Piddington <kpiddington>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, cmarcelo, darin, dino, dm, eric.carlson, ews-watchlist, felix.herbst, glenn, gyuyoung.kim, Hironori.Fujii, jbedard, jer.noble, jhurliman, jonlee, kbr, kkinnunen, kondapallykalyan, kpiddington, luiz, philipj, remi, ryanhaddad, ryuan.choi, sergio, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=233964
https://bugs.webkit.org/show_bug.cgi?id=233965
Bug Depends on: 232064, 228240, 233279    
Bug Blocks: 232367, 233007, 233825, 218949, 220076, 233817, 233837, 233952, 235278, 235281, 236427, 236699, 236733    
Attachments:
Description Flags
changes.diff from experimental rebase
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Fix WinCairo build
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch none

Dean Jackson
Reported 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.
Attachments
changes.diff from experimental rebase (377.94 KB, patch)
2021-11-08 17:24 PST, Kyle Piddington
no flags
Patch (41.41 MB, patch)
2021-11-30 13:55 PST, Kyle Piddington
no flags
Patch (41.41 MB, patch)
2021-11-30 17:37 PST, Kyle Piddington
ews-feeder: commit-queue-
Patch (41.43 MB, patch)
2021-12-01 16:54 PST, Kyle Piddington
ews-feeder: commit-queue-
Fix WinCairo build (11.20 KB, patch)
2021-12-01 18:21 PST, Fujii Hironori
no flags
Patch (41.43 MB, patch)
2021-12-02 22:47 PST, Kyle Piddington
ews-feeder: commit-queue-
Patch (41.43 MB, patch)
2021-12-02 23:52 PST, Kyle Piddington
ews-feeder: commit-queue-
Patch (41.44 MB, patch)
2021-12-03 13:36 PST, Kyle Piddington
ews-feeder: commit-queue-
Patch (41.44 MB, patch)
2021-12-06 17:08 PST, Kyle Piddington
no flags
Patch (41.44 MB, patch)
2021-12-06 18:48 PST, Kyle Piddington
no flags
Radar WebKit Bug Importer
Comment 1 2021-01-23 14:09:26 PST
Kenneth Russell
Comment 2 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.
Kenneth Russell
Comment 3 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.
Kyle Piddington
Comment 4 2021-11-08 17:24:03 PST
Created attachment 443636 [details] changes.diff from experimental rebase
Kyle Piddington
Comment 5 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?
Kyle Piddington
Comment 6 2021-11-30 13:55:40 PST
Kyle Piddington
Comment 7 2021-11-30 17:37:51 PST
EWS Watchlist
Comment 8 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
Kyle Piddington
Comment 9 2021-12-01 16:54:43 PST
Fujii Hironori
Comment 10 2021-12-01 18:21:29 PST
Created attachment 445652 [details] Fix WinCairo build Please merge this fix for your next patch.
Kyle Piddington
Comment 11 2021-12-02 22:47:43 PST
Kyle Piddington
Comment 12 2021-12-02 23:52:02 PST
Kyle Piddington
Comment 13 2021-12-03 13:36:28 PST
Kyle Piddington
Comment 14 2021-12-06 17:08:03 PST
Kyle Piddington
Comment 15 2021-12-06 18:48:36 PST
Kenneth Russell
Comment 16 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.
Dean Jackson
Comment 17 2021-12-07 11:52:30 PST
Committed revision 286603.
Dean Jackson
Comment 18 2021-12-07 11:53:15 PST
Kenneth Russell
Comment 19 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!
Yusuke Suzuki
Comment 20 2021-12-07 18:50:06 PST
Kenneth Russell
Comment 21 2021-12-15 11:39:48 PST
*** Bug 233817 has been marked as a duplicate of this bug. ***
Kyle Piddington
Comment 22 2022-01-19 11:15:21 PST
*** Bug 232905 has been marked as a duplicate of this bug. ***
Kyle Piddington
Comment 23 2022-02-03 15:41:40 PST
*** Bug 230216 has been marked as a duplicate of this bug. ***
Darin Adler
Comment 24 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
Note You need to log in before you can comment on or make changes to this bug.