search

KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
3.07k stars 842 forks source link

SPV Remapper: should use SPIRV-Tools binary parser #474

Open ghost opened 8 years ago

ghost commented 8 years ago

I thought this should be logged as a separate issue from SPV Remapper testing (in progress), since it's not really coupled to that, and is likely to be addressed separately.

There is a duplication of SPIR-V binary parsing. The remapper has one, and there is a more generic one in SPIRV-Tools. The remapper should use the SPIRV-Tools parser. Since the latter treats the binary as const during traversal, this will mean a little bit of redesign to avoid any in-place modifications. That's resolvable by building another binary on the fly, and swapping them back and forth between modification passes.

sheredom commented 7 years ago

@steve-lunarg (revisiting an old issue here) - it'd make more sense in my opinion that the functionality of the spirv remapper be moved into spirv-opt as an optimization pass. I know remapping IDs isn't technically optimizing the resultant binary in any fashion, but it does optimize the shipping process of SPIR-V shaders. Thoughts?

ghost commented 7 years ago

Hi @sheredom,

(Same person you're replying to, different account for personal context separation purposes): I imagine two aspects to this, which may or may not have the same answer:

  1. The spirv-remap tool has some low-quality classical optimizations to facilitate size reduction, such as a simple load/store eliminator. Now that spirv-opt exists, I think those optimizations should be removed from the remapper. Spirv-opt will do a better job anyway. There might be a bit of inertia around this (e.g, the remapper built into user tool chains that are not yet using spirv-opt), but that's an argument for a deprecation period and clear communication, not an argument against removal in favor of spirv-opt.

  2. ID remapping: IMHO this would be better moved to the SPIRV-Tools framework in some capacity. Whether that best lands in spirv-opt, or as its own tool, I'm not sure, and anyway it would need discussion with the SPIRV-Tools folks. I'm not opposed to it being a pass, if that makes sense.

I suppose depending on how (2) falls out, that either leaves the remapper as a smaller and simpler standalone tool, or as a spirv-opt pass.

Related: the current remapping process was built to a specific set of requirements, but generalizations of several of those are possible. For example, the questions: "is a post decompression step to recover valid SPIRV acceptable", and "is cross-module input analysis acceptable" form a small state space. (There might be another one or two like that). Present tool lands in "no/no", but the other combinations are valid and beneficial if user circumstances allow them.

greg-lunarg commented 1 year ago

FWIW, the maintenance burden of the parser in spirv-remap has historically been next to null. Likewise for the --dce and --opt options. Making any changes at this moment with the subsequent compatibility issues and risk of regression does not seem warranted.