emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.75k stars 3.3k forks source link

library_webgpu.js: the setBindGroup shim can be simplified. #20280

Open floooh opened 1 year ago

floooh commented 1 year ago

The current setBindGroup shims in library_webgpu.js create an adhoc JS array object when called with dynamicOffsetCount > 0:

https://github.com/emscripten-core/emscripten/blob/0be5b609412d5b07f8157528df8e5088dae84858/src/library_webgpu.js#L2126C3-L2138

  wgpuRenderPassEncoderSetBindGroup: (passId, groupIndex, groupId, dynamicOffsetCount, dynamicOffsetsPtr) => {
    var pass = WebGPU.mgrRenderPassEncoder.get(passId);
    var group = WebGPU.mgrBindGroup.get(groupId);
    if (dynamicOffsetCount == 0) {
      pass["setBindGroup"](groupIndex, group);
    } else {
      var offsets = [];
      for (var i = 0; i < dynamicOffsetCount; i++, dynamicOffsetsPtr += 4) {
        offsets.push({{{ gpu.makeGetU32('dynamicOffsetsPtr', 0) }}});
      }
      pass["setBindGroup"](groupIndex, group, offsets);
    }
  },

By using a different setBindGroup overload, this can be simplified to:

  wgpuRenderPassEncoderSetBindGroup: (passId, groupIndex, groupId, dynamicOffsetCount, dynamicOffsetsPtr) => {
    var pass = WebGPU.mgrRenderPassEncoder.get(passId);
    var group = WebGPU.mgrBindGroup.get(groupId);
    if (dynamicOffsetCount == 0) {
      pass["setBindGroup"](groupIndex, group);
    } else {
      pass["setBindGroup"](groupIndex, group, HEAPU32, dynamicOffsetsPtr>>2, dynamicOffsetCount);
    }
  },

Tested on an M1 Mac on Chrome - I'm actually not sure if the previous code is just a leftover before this separate overload function existed, or whether this is because of compatibility with non-Chrome browsers - maybe their WebGPU implementation hasn't caught up yet).

Also, the same fix should be applied to the other setBindGroup variants:

(cc: @kainino0x)

kainino0x commented 1 year ago

I suspect this is just leftover from before we added the overload, though unfortunately it does look like Firefox doesn't support it. https://searchfox.org/mozilla-central/source/dom/webidl/WebGPU.webidl#907

Perhaps we can hold off since I don't think this is very important to fix now.

kainino0x commented 8 months ago

Pull request: https://github.com/emscripten-core/emscripten/pull/21221