Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented May 19, 2023

Creates a reusable function macro for performing a prefix sum. Eventually, we'll need this for polyline decomposition

image

Reasonably fast with max (1024) input elements.

@jonahwilliams jonahwilliams requested a review from dnfield May 19, 2023 18:38
reinterpret_cast<CS::OutputData<kCount>*>(view.contents);
EXPECT_TRUE(output);

constexpr uint32_t expected[kCount + 1] = {0, 1, 3, 6, 10, 15};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should really start at 1, right? or should I be expected to shift the sequence in the shader

// Parallel prefix sum computes the prefix in place in storage.
// BLOCK_SIZE is the overall storage size while ident must be the global
// x identifier.
#define PrefixSum(ident, storage, BLOCK_SIZE) \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a simple and not particularly fast implementation, but its good enough to start with and could be swapped for a more efficient implementation later.

Copy link
Contributor

Choose a reason for hiding this comment

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

This won't be as efficient as the hardware implementations (e.g. subgroupInclusive/ExclusiveAdd).

We also should define whether this is exclusive or inclusive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking we could if/def or function constant our way into more efficient implementations. I have more reading to do though. Where should I look into the subgroupInclusive/ExclusiveAdd?

@jonahwilliams
Copy link
Contributor Author

Ahh the issue was that this was an exclusive sum and I need to convert that to an inclusive sum. Fixed

@chinmaygarde chinmaygarde self-requested a review May 21, 2023 19:58
// Parallel exclusive prefix sum computes the prefix in place in storage.
// BLOCK_SIZE is the overall storage size while ident must be the global
// x identifier.
#define ExclusivePrefixSum(ident, storage, BLOCK_SIZE) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a macro and not a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs to work over threadgroup shared memory which can't be passed as a ptr in glsl as far as I can tell

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, GLSL doesn't have pointers, you can't pass shared memory to a function defined in another file (it will try to do a copy)

@chinmaygarde chinmaygarde changed the title [Impeller] create reusable prefix sum [Impeller] Create reusable prefix sum. May 21, 2023
@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label May 22, 2023
@auto-submit auto-submit bot merged commit 82c30a0 into flutter:main May 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 22, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 22, 2023
flutter/engine@1ed9fc0...a342a91

2023-05-22 skia-flutter-autoroll@skia.org Roll Skia from 2612bb159848 to d448fe07ea46 (5 revisions) (flutter/engine#42213)
2023-05-22 aam@google.com Skip and ignore boringssl/src/rust when looking for the licenses. (flutter/engine#42210)
2023-05-22 jonahwilliams@google.com [Impeller] Create reusable prefix sum. (flutter/engine#42167)

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 rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
…7352)

flutter/engine@1ed9fc0...a342a91

2023-05-22 skia-flutter-autoroll@skia.org Roll Skia from 2612bb159848 to d448fe07ea46 (5 revisions) (flutter/engine#42213)
2023-05-22 aam@google.com Skip and ignore boringssl/src/rust when looking for the licenses. (flutter/engine#42210)
2023-05-22 jonahwilliams@google.com [Impeller] Create reusable prefix sum. (flutter/engine#42167)

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 rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants