[Impeller] add compute pass API for memory barriers, re-enable for Vulkan. #49946

jonahwilliams · 2024-01-22T20:22:52Z

Adds two new APIs that insert memory barriers for compute -> compute dependencies.

  // |ComputePass|
  void AddBufferMemoryBarrier() override;

  // |ComputePass|
  void AddTextureMemoryBarrier() override;

Also makes the ComputePassVK automatically insert a compute -> vertex dependency when encoding. This change is sufficient to let the GPU compute implementation of draw points work on Pixel and Samsung Android devices.

For more explaination on these specific barriers, see the documentation added in this PR.

Fixes #49946

…lkan

jonahwilliams · 2024-01-23T00:18:24Z

impeller/renderer/backend/metal/compute_pass_mtl.mm

@@ -32,7 +32,8 @@
  if (!buffer_) {
    return;
  }
-  encoder_ = [buffer_ computeCommandEncoder];
+  encoder_ = [buffer_ computeCommandEncoderWithDispatchType:


By default we were creating a serial encoder, which acts like it has a global memory barrier between each command. Since we need to specify the barriers for Vulkan, we might as well use the concurrent dispatcher for iOS too.

jonahwilliams · 2024-01-23T00:56:24Z

impeller/renderer/compute_unittests.cc

@@ -308,6 +308,7 @@ TEST_P(ComputeTest, MultiStageInputAndOutput) {
    CS1::BindOutput(*pass, DeviceBuffer::AsBufferView(output_buffer_1));

    ASSERT_TRUE(pass->Compute(ISize(512, 1)).ok());
+    pass->AddBufferMemoryBarrier();


Since we're now running concurrently, we need a memory barrier here so that step 2 sees the output of step 1.

matanlurey

Comments and explanations look really good. Two notes.

Not an explicit approval as someone who understands what a memory barrier and whether it's being used properly should do that step.

matanlurey · 2024-01-23T01:04:53Z

impeller/entity/geometry/point_field_geometry.cc

@@ -267,9 +268,7 @@ GeometryVertexType PointFieldGeometry::GetVertexType() const {
 // Compute is disabled for Vulkan because the barriers are incorrect, see


Is this out of date with your change?

Yes, removed.

matanlurey · 2024-01-23T01:06:56Z

impeller/renderer/compute_pass.h

+  ///        This only handles read after write hazards.
+  virtual void AddBufferMemoryBarrier() = 0;
+
+  /// @brief Add a barrier that ensures all previously encoded commands have


Try to use the same verbage on similar API, otherwise my brain thinks "hmm this one Adds a barrier" but Buffer memory doesn't? (I'm guessing they both could be written more similar, and if not, let's clarify)

is the adds a barrier part redundant? I should update this too, I was trying to describe what a barrier does instead of restating the method name.

I think "ensures all previously encoded ..." is probably sufficient.

The fact that it does so by adding the barrier is, as you stated, is in the API name.

The only thing I'd add I suppose is when this API is supposed to be used.

Rephrased what this does.

chinmaygarde

I believe the barriers are underspecified in Vulkan and the Metal and Vulkan semantics are different for the transfer and compute/render dependencies.

Other than that, some nits on documentation.

chinmaygarde · 2024-01-23T02:31:55Z

impeller/renderer/compute_pass.h

+  ///        matter if the compute command is in a different command buffer.
+  virtual void AddBufferMemoryBarrier() = 0;
+
+  /// @brief Ensures all previously encoded commands have finished writing to


Minor edit for clarity: "Ensures that side effects (to textures) of all commands previously encoded in the pass are visible to subsequent commands in the pass."

Since "finished writing" isn't quite the same as having those writes be visible to subsequent commands. Because of caches and such.

chinmaygarde · 2024-01-23T02:35:47Z

impeller/renderer/compute_pass.h

+  ///        textures before any following commands can read from those
+  ///        textures.
+  ///
+  ///        This method after encoding a command that writes to a storage


This sentence is quite confusing to me. Did you mean "Call this method?" as a recommended usage?

Yeah removed, I don't know what I was doing here

chinmaygarde · 2024-01-23T02:45:42Z

impeller/renderer/backend/vulkan/compute_pass_vk.cc

+// |ComputePass|
+void ComputePassVK::AddBufferMemoryBarrier() {
+  vk::MemoryBarrier barrier;
+  barrier.srcAccessMask = vk::AccessFlagBits::eShaderWrite;


These to me seem underspecified. Right now, this says "All commands before the barrier must have buffer-writes in their compute shaders be visible to buffer-reads in also the compute shaders of commands after the barrier".

The same can apply to transfer read-write (VK_ACCESS_TRANSFER_READ|WRITE_BIT in VK_PIPELINE_STAGE_TRANSFER_BIT) in the transfer stage in addition to any compute-render dependencies.

You can just or them up. But I suggest a comment indicating each value.

This would also match up with Metal semantics implemented above.

Why would I need to specify transfer dependencies in a barrier API that is specifically for compute-compute dependencies?

I think I clarified on the API docs that it is only for compute-compute dependencies, but I can add a caveat that it does not cover other cases.

Fair. But the first compute pass still needs to wait for say transfer writes on the buffers it works on to be visible. So my expectation would be that compute-compute dependencies while necessary are not sufficient.

We don't use transfer commands anywhere yet though. my expectation is that if we needed this we would add this blit/transfer -> compute barrier functionality to the blit pass and not to the compute pass.

chinmaygarde · 2024-01-23T02:46:59Z

impeller/renderer/compute_pass.h

+  ///        This method after encoding a command that writes to a storage
+  ///        texture that a subsequent command will read from, provided that
+  ///        is a compute command and not a render pass command. It does not
+  ///        matter if the compute command is in a different command buffer.


Different command buffer but still subject to queue submission order right? Perhaps I am being pedantic.

Right, done. Expect of course on metal where the command buffers are themselves a unit of synchronization.

chinmaygarde · 2024-01-23T02:48:41Z

impeller/renderer/backend/vulkan/compute_pass_vk.cc

@@ -213,8 +213,53 @@ bool ComputePassVK::BindResource(size_t binding,
  return true;
 }

+// Note:
+// https://github.com/KhronosGroup/Vulkan-Docs/wiki/Synchronization-Examples


I don't see how that is the case. Though you do need the buffer and texture references to specify the barriers on. So this approach is definitely more easy to use if coarse.

We could easily add these to the API and create the more specific barriers, but given that we dont have any way to evaluate it this yet, I'd prefer to follow the recommendations.

flutter-dashboard · 2024-01-23T19:49:32Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #49946 at sha e9adf67

dnfield · 2024-01-23T20:20:50Z

That golden change looks wrong?

jonahwilliams · 2024-01-23T20:41:16Z

Yes it does. Let me double check this, we might need one more barrier.

flutter-dashboard · 2024-01-25T21:11:16Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

…lkan

jonahwilliams · 2024-01-26T18:20:57Z

Changing to non-draft for golden updates, still WIP

flutter-dashboard · 2024-01-26T19:00:25Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #49946 at sha 303f1e7

flutter-dashboard · 2024-01-26T19:42:42Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #49946 at sha 18f6f54

jonahwilliams · 2024-01-26T20:23:38Z

impeller/renderer/backend/vulkan/compute_pass_vk.cc

-        static_cast<int64_t>(std::ceil(width * 1.0 / max_wg_size_[0] * 1.0)),
-        minimum);
-    command_buffer_vk.dispatch(threadGroups, 1, 1);
+    command_buffer_vk.dispatch(width, 1, 1);


This logic was incorrect...

…ble for Vulkan. (flutter/engine#49946)

…142347) flutter/engine@525bd7d...a65a1b5 2024-01-26 [email protected] Roll Skia from c32aa37effcc to 6279c88b9e29 (1 revision) (flutter/engine#50098) 2024-01-26 [email protected] [Impeller] add compute pass API for memory barriers, re-enable for Vulkan. (flutter/engine#49946) 2024-01-26 [email protected] Introduce a prototype of a "header guard enforcement" tool (flutter/engine#48903) 2024-01-26 [email protected] Roll Skia from e24124912cc3 to c32aa37effcc (1 revision) (flutter/engine#50094) 2024-01-26 [email protected] [web] Do not wipe the PlatformViewManager when disposing of a view. (flutter/engine#49991) 2024-01-26 [email protected] Finish landing missing/incorrect header guards across `flutter/engine` (flutter/engine#50069) 2024-01-26 [email protected] Roll Skia from 32f6bff0f193 to e24124912cc3 (2 revisions) (flutter/engine#50093) 2024-01-26 [email protected] Roll Skia from cbdf09d69efc to 32f6bff0f193 (3 revisions) (flutter/engine#50092) 2024-01-26 [email protected] Roll Dart SDK from 5636e338e0b9 to 7337995bc851 (1 revision) (flutter/engine#50087) 2024-01-26 [email protected] Fix Shell::Screenshot for Impeller (flutter/engine#50072) 2024-01-26 [email protected] Roll Skia from ae73baacb793 to cbdf09d69efc (1 revision) (flutter/engine#50085) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

jonahwilliams added 4 commits January 20, 2024 14:10

[Impeller] dont emulate command buffers for compute on Metal/Vulkan.

845e8f6

[Impeller] fix compute barriers for Vulkan.

94e274e

use old synchronization.

e049730

fix stage masks.

30e474c

github-actions bot added the e: impeller label Jan 22, 2024

jonahwilliams added 3 commits January 22, 2024 16:13

Merge branch 'main' of github.com:flutter/engine into fix_barriers_vu…

2b4c3bc

…lkan

cleanup

802de21

revert context changes.

ec94606

jonahwilliams commented Jan 23, 2024

View reviewed changes

jonahwilliams changed the title ~~Fix barriers vulkan~~ [Impeller] add compute pass API for memory barriers, re-enable for Vulkan. Jan 23, 2024

remove unused argument, add barrier.

e5f44ce

jonahwilliams marked this pull request as ready for review January 23, 2024 00:55

jonahwilliams commented Jan 23, 2024

View reviewed changes

jonahwilliams requested review from chinmaygarde and dnfield January 23, 2024 00:58

matanlurey reviewed Jan 23, 2024

View reviewed changes

jonahwilliams added 2 commits January 22, 2024 17:11

fix doc comment.

0c2adf5

++

0ec1f76

chinmaygarde reviewed Jan 23, 2024

View reviewed changes

jonahwilliams added 2 commits January 22, 2024 19:28

++

555e92d

formatting.

8220ebf

chinmaygarde approved these changes Jan 23, 2024

View reviewed changes

Merge branch 'main' into fix_barriers_vulkan

e9adf67

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 23, 2024

jonahwilliams mentioned this pull request Jan 23, 2024

[Impeller] HAL needs barriers for both Metal/Vulkan for compute usage. flutter/flutter#140798

Closed

flutter-dashboard bot added the will affect goldens label Jan 23, 2024

jonahwilliams removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 23, 2024

jonahwilliams marked this pull request as draft January 25, 2024 21:05

Merge branch 'main' of github.com:flutter/engine into fix_barriers_vu…

303f1e7

…lkan

jonahwilliams marked this pull request as ready for review January 26, 2024 18:21

test

18f6f54

jonahwilliams commented Jan 26, 2024

View reviewed changes

chinmaygarde approved these changes Jan 26, 2024

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 26, 2024

auto-submit bot merged commit d27b18d into flutter:main Jan 26, 2024

engine-flutter-autoroll mentioned this pull request Jan 26, 2024

Roll Flutter Engine from 525bd7dcf7f3 to d27b18d912d7 (10 revisions) flutter/flutter#142346

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2024

d27b18d91 [Impeller] add compute pass API for memory barriers, re-ena…

0dd4fe4

…ble for Vulkan. (flutter/engine#49946)

engine-flutter-autoroll mentioned this pull request Jan 26, 2024

Roll Flutter Engine from 525bd7dcf7f3 to a65a1b55e06a (11 revisions) flutter/flutter#142347

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2024

d27b18d91 [Impeller] add compute pass API for memory barriers, re-ena…

f35bde9

…ble for Vulkan. (flutter/engine#49946)

		@@ -267,9 +268,7 @@ GeometryVertexType PointFieldGeometry::GetVertexType() const {
		// Compute is disabled for Vulkan because the barriers are incorrect, see

[Impeller] add compute pass API for memory barriers, re-enable for Vulkan. #49946

[Impeller] add compute pass API for memory barriers, re-enable for Vulkan. #49946

Uh oh!

Conversation

jonahwilliams commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matanlurey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Jan 23, 2024

Uh oh!

dnfield commented Jan 23, 2024

Uh oh!

jonahwilliams commented Jan 23, 2024

Uh oh!

flutter-dashboard bot commented Jan 25, 2024

Uh oh!

jonahwilliams commented Jan 26, 2024

Uh oh!

flutter-dashboard bot commented Jan 26, 2024

Uh oh!

flutter-dashboard bot commented Jan 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahwilliams commented Jan 22, 2024 •

edited

Loading