engine icon indicating copy to clipboard operation
engine copied to clipboard

[Impeller] Improvements for SSBO codegen

Open dnfield opened this issue 3 years ago • 8 comments

This PR does a few things:

  • Adds a UintPoint32 and a IPoint32 to handle uvec2/ivec2 members, and updates the reflector to know about these.
  • Turns structs that have dynamically sized arrays (aka flexible arrays) into template classes that take a size parameter.
    • This allows for stack allocation instead of forcing heap allocation.
    • This allows for structs that have more than just the array member in GLSL.
    • This requires updating HostBuffer::EmplaceStorageBuffer
  • Turns the array_elements member of the struct definition into an optional so that callsites can decide what to do if there's no array elements instead of forcing them to always think there's at least 1.
  • Updates the compute test/sample.comp to use these features.
  • Updates a bug in the way we handled array sizes. Previously we would have taken a multi-dimensional array and flattened it out as if single dimensional. Impellerc will now enforce that you don't use multi-dimensional arrays in GLSL, since it's not supported everywhere we need (SkSL, GLES 2.0 for example).

Fixes https://github.com/flutter/flutter/issues/110618 Addresses part of the concerns in https://github.com/flutter/flutter/issues/112683

dnfield avatar Oct 05 '22 17:10 dnfield

This might have some unintentional impact on UBOs but all tests are passing so I'm hoping @bdero can spot that if it's a problem.

dnfield avatar Oct 05 '22 17:10 dnfield

All tests are passing

lol I spoke too soon. Will look into tests.

dnfield avatar Oct 05 '22 17:10 dnfield

These two make for a pretty good smoke test of uniform breaking:

  • RendererTest.InactiveUniforms should show up as solid green on Metal and GLES.
  • RendererTest.ArrayUniforms should animate 4 moving dots on Metal and GLES.

Screen Shot 2022-10-05 at 12 02 29 PM

bdero avatar Oct 05 '22 19:10 bdero

These two make for a pretty good smoke test of uniform breaking:

  • RendererTest.InactiveUniforms should show up as solid green on Metal and GLES.
  • RendererTest.ArrayUniforms should animate 4 moving dots on Metal and GLES.

Yes, they're both looking the same to me.

dnfield avatar Oct 05 '22 19:10 dnfield

I talked to @zanderso - we had a test that was exercising defining a 2d array in a shader, but the usage of it was a bit odd. I'm amending the test to drop the 2d array.

dnfield avatar Oct 05 '22 19:10 dnfield

There's some kind of race going on here. If I run the tests locally with cpulimit -l 1they sometimes fail the way CI is showing, other times not. Moving to draft until I fix that.

dnfield avatar Oct 05 '22 21:10 dnfield

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.

flutter-dashboard[bot] avatar Oct 05 '22 21:10 flutter-dashboard[bot]

I looked through https://developer.apple.com/documentation/metal/performing_calculations_on_a_gpu?language=objc again and the only meaningful difference I could find was that I was setting a much larger (and two dimensional) grid and workgroup size. If I set it to something more appropriately sized (exactly sized to my data in this case), it doesn't seem to flake locally after a few thousand runs with a cpu limiter (whereas before it would flake fairly often).

I'm not thrilled with this but it seems like a bug on the Apple side to me.

dnfield avatar Oct 06 '22 00:10 dnfield