Vectorize rrect_blur

[gaaclarke]

issue: flutter/flutter#148496

28% speed improvement in the rrect_blur shader.

before

after

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[chinmaygarde]

Really curious to see the malioc diff.

[gaaclarke]

Really curious to see the malioc diff.

Here's one from before my last (and final) vectorization: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8735240889996581441/+/u/test:_malioc_diff/stdout

[gaaclarke]

One thing that is unfortunate is that it seems to falling into "stack spilling". I would have expected less or equal register usage. It possibly could be faster if we could avoid that.

[chinmaygarde]

The stack spills are concerning. And thats happening on Mali-T880 which is our low-end representative. Perhaps this makes things faster all the way till we run out of stack space.

It's faster on iOS because there are likely no spills.

[gaaclarke]

It's faster on iOS because there are likely no spills.

Or there is spilling on iOS too but the savings from the vectorization are larger than the load/store calls. I could try to reuse variables more to see if that reduces register pressure but I can't run malioc locally. Do we have an easy way to do that? I have it installed locally but it doesn't handle includes and doesn't seem to have an argument to make them work.

$ /Applications/Arm_Performance_Studio_2024.3/mali_offline_compiler/malioc --opengles --fragment --core Mali-G78 impeller/entity/shaders/rrect_blur.frag 
Mali Offline Compiler v8.4.1 (Build 215478)
Copyright (c) 2007-2024 Arm Limited. All rights reserved.

Configuration
=============

Hardware: Mali-G78 r1p1
Architecture: Valhall
Driver: r48p0-00rel0
Shader type: OpenGL ES Fragment

Compiler messages
=================

ERROR: 0:7: P0001: Unknown preprocessing directive 'include'
     
  >> #include <impeller/gaussian.glsl>
     #include <impeller/types.glsl>

ERROR: 0:8: P0001: Unknown preprocessing directive 'include'
     #include <impeller/gaussian.glsl>
  >> #include <impeller/types.glsl>
     
ERROR: 0:10: L0001: Typename expected, found 'FragInfo'
     
  >> uniform FragInfo {
       f16vec4 color;

ERROR: 0:10: L0001: Expected identifier, found '{'
     
  >> uniform FragInfo {
       f16vec4 color;

Compilation failed.

[gaaclarke]

For mali-t880 longest/shortest path, (14.85 + 9.0) < (25.74 + 1.0) so it still seems like a net positive.

[chinmaygarde]

If you have malioc installed locally, then it should be possible to just run the same script the bots are running to generate the diff. If malioc not in your path perhaps?

[chinmaygarde]

My sense was that a stack spill was an instakill to performance. And so, I don't think we have any other shaders that spill the stack even on low end devices. I do not think you are seeing any spills on iOS but we could verify.

But, I think this may still be fine. Thanks to you your patch, the arith unit is more hungry now. Hungry enough that the data can't fit the stack (on low-end devices). So the load-store unit does more work to ferry stuff between the stack and memory. But the shader is still arith bound. Perhaps my intuition about spills being bad is misinformed and based on cases where the spills cause the shader to be load-store bound?

I'll ask around for some guidance since I do feel skill-checked here. I'd feel more comfortable doing this only on iOS or testing on the representative low-end device. I'll also followup on more informed guidance on how to interpret this.

Either way, nicely done! Good spot on the loop count fitting in a vector.

[jonahwilliams]

So I'd be careful with vectorization. Most GPUs are SIMT now, and so despite data types being vec4 based all of the math ops are still scalar.

My sense was that a stack spill was an instakill to performance.

This is correct, a stack spill is absolutely catastrophic for performance.

[jonahwilliams]

I would like to spend some time verifying the reported performance numbers. I had previously investigate this and on iOS found that it was net negative.

[gaaclarke]

I got rid of the stack spill by reusing the integral variable:

$ malioc --fragment --core Mali-T880 ../out/host_debug_unopt_arm64/gen/flutter/impeller/entity/gles/rrect_blur.frag.gles
Mali Offline Compiler v8.4.1 (Build 215478)
Copyright (c) 2007-2024 Arm Limited. All rights reserved.

Configuration
=============

Hardware: Mali-T880 r2p0
Architecture: Midgard
Driver: r23p0-00rel0
Shader type: OpenGL ES Fragment

Main shader
===========

Work registers: 7 (87% used at 50% occupancy)
Uniform registers: 3 (17% used)
Stack spilling: false

                                A      LS       T    Bound
Total instruction cycles:   13.33    1.00    0.00        A
Shortest path cycles:       12.87    1.00    0.00        A
Longest path cycles:        12.87    1.00    0.00        A

A = Arithmetic, LS = Load/Store, T = Texture

Shader properties
=================

Has uniform computation: false

Note: This tool shows only the shader-visible property state.
API configuration may also impact the value of some properties.

[jonahwilliams]

Numbers check out locally for me. Good to hear that you got rid of the stack spill!

LGTM

[chinmaygarde]

I got rid of the stack spill by reusing the integral variable

Awesome. I have no concerns past that. Seems like Jonah had some prior experience not seeing expected results with vectorization so get a +1 from him as well.

[chinmaygarde]

Numbers check out locally for me.

Nevermind 🙂

[auto-submit]

auto label is removed for flutter/engine/55576, due to - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

[flutter-dashboard]

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 #55576 at sha cbb3198

[chinmaygarde]

Hmm, the edges don't look quite right

[gaaclarke]

The error was introduced in 56a32c7 when i tried to avoid the stack spilling. Let me double check that diff.

[gaaclarke]

fixed, I got my swizzles swizzled