KhronosGroup / glslang

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

Provide option to skip default binding decoration, or a mechanism for tracking where the defaults have been set. #2218

Open nitronoid opened 4 years ago

nitronoid commented 4 years ago

Quoting from my previous comment in an older issue:

"The tool I'm working on is feeding ESSL through and producing SPIRV, with the intention to patch any blocks which haven't been given explicit bindings by generating them ourselves. This becomes difficult with the changes in the PR you referenced as a binding of 0 is perfectly valid and offers no way of knowing that this uniform wasn't given an explicit binding.

I'm wondering if you have any suggestions on how I could work around this problem? Currently I have subclassed TGlslIoMapper to modify the binding values when not explicitly provided to a sentinel value which we check for later, i.e.

if (!qualifier.hasBinding()) qualifier.layoutBinding = sentinel; But this feels like a hack and may be fragile.

Perhaps instead of just setting the binding to zero, these uniforms could also be tracked by the shader so we can at least tell where default values have been assigned?"

It was suggested that the option --shift-ssbo-binding could be of use. Does this option apply to all ssbos or just the ones that have been given the default binding of 0. In any case, this is a possible way to recreate the work around I am already using, when the ideal case would be to avoid decorating them with a binding.

Edit

The PR mentioned above is this one: https://github.com/KhronosGroup/glslang/pull/1625

RuoyuAMD commented 4 years ago

See StandAlone.cpp line: 1115 if (! Program.mapIO ()) If you want use TGlslIoMapper you should use TGlslIoMapper and TDefaultGlslIoResolver as the function (mapIO) parameter, it's beta code, we only use it in Opengl driver. so glslangValidator won't be effect by this class and this rule, If you want try, you can modify the code in local. When we finished develop plan and the test case is done, we will add it to glslangValidator and use new option to control it.

nitronoid commented 4 years ago

To be clear, what I really want is for no IO mapping to be performed. Simply compiling the glsl shader gives me very close to what I want, except for the OpDecoration Binding 0 being applied to everything. The only reason I am using an IoMapper at all is to set the binding value for anything which doesn't have one in advance to prevent it from being defaulted to zero.

This is the code I am using:

class DummyMapper : public glslang::TGlslIoMapper
{
public:
    DummyMapper (EShLanguage stage) : glslang::TGlslIoMapper{}, stage{ stage } {}
    EShLanguage stage;

    virtual bool doMap(glslang::TIoMapResolver* resolver, TInfoSink& info) override
    {
        assert(uniformVarMap[stage] && "null uniform map\n");
        // Set bindings to max allowed value to avoid being set to 0 by default
        for (auto& p : *uniformVarMap[stage])
        {
            auto& ent = p.second;
            auto& type = ent.symbol->getWritableType();
            auto& qual = type.getQualifier();
            if (!qual.hasBinding())
            {
                // Tricky way of setting entire bitfield, need to init to zero first to ensure no UB
                qual.layoutBinding = 0u;
                qual.layoutBinding = ~qual.layoutBinding - 1;
            }
        }
        return true;
    }
};

...

glslang::TShader shader{stage};

...

if (!shader.parse(&resources, default_version, false, messages))
{
    std::cerr << "glslang compile failed.\n\n" << shader.getInfoLog() << '\n' << shader.getInfoDebugLog();
}

DummyMapper mapper{stage};
glslang::TProgram program;
program.addShader(&shader);
if (!(program.link(messages) && program.mapIO(nullptr, &mapper)))
{
    std::cerr << "glslang link failed.\n\n" << program.getInfoLog() << '\n' << program.getInfoDebugLog();
}

Which will max out the binding value, so we can identify bindings which were not set in the shader.

RuoyuAMD commented 4 years ago

To be clear, what I really want is for no IO mapping to be performed. Simply compiling the glsl shader gives me very close to what I want, except for the OpDecoration Binding 0 being applied to everything. The only reason I am using an IoMapper at all is to set the binding value for anything which doesn't have one in advance to prevent it from being defaulted to zero.

This is the code I am using:

class DummyMapper : public glslang::TGlslIoMapper
{
public:
  DummyMapper (EShLanguage stage) : glslang::TGlslIoMapper{}, stage{ stage } {}
  EShLanguage stage;

  virtual bool doMap(glslang::TIoMapResolver* resolver, TInfoSink& info) override
  {
      assert(uniformVarMap[stage] && "null uniform map\n");
      // Set bindings to max allowed value to avoid being set to 0 by default
      for (auto& p : *uniformVarMap[stage])
      {
          auto& ent = p.second;
          auto& type = ent.symbol->getWritableType();
          auto& qual = type.getQualifier();
          if (!qual.hasBinding())
          {
              // Tricky way of setting entire bitfield, need to init to zero first to ensure no UB
              qual.layoutBinding = 0u;
              qual.layoutBinding = ~qual.layoutBinding - 1;
          }
      }
      return true;
  }
};

...

glslang::TShader shader{stage};

...

if (!shader.parse(&resources, default_version, false, messages))
{
  std::cerr << "glslang compile failed.\n\n" << shader.getInfoLog() << '\n' << shader.getInfoDebugLog();
}

DummyMapper mapper{stage};
glslang::TProgram program;
program.addShader(&shader);
if (!(program.link(messages) && program.mapIO(nullptr, &mapper)))
{
  std::cerr << "glslang link failed.\n\n" << program.getInfoLog() << '\n' << program.getInfoDebugLog();
}

Which will max out the binding value, so we can identify bindings which were not set in the shader.

Your code has implemented these functions, if you want to know what binding is specified in the shader. I have not fully understood why you need to mark the binding specified in the shader, but if you not do io map, no binding variable will default to -1. And I don't think it fragile, before iomap, AST stored very reliable raw data.

nitronoid commented 4 years ago

What you say is true, however in GlslangToSpv bindings with a value of -1 get set to zero, meaning that we lose this information. This is why I manually max out the binding value. The problem with maxing out the value in this way is that the "max" is subject to the type which you use internally to store the binding value. It is also a bitfield, so the max value changes if you were to modify the width. I would be happy enough if the -1 was not replaced by a zero when converting to SPIRV, or if no decoration was output.

RuoyuAMD commented 4 years ago

What you say is true, however in GlslangToSpv bindings with a value of -1 get set to zero, meaning that we lose this information. This is why I manually max out the binding value. The problem with maxing out the value in this way is that the "max" is subject to the type which you use internally to store the binding value. It is also a bitfield, so the max value changes if you were to modify the width. I would be happy enough if the -1 was not replaced by a zero when converting to SPIRV, or if no decoration was output.

I think for this problem, we can consider adding an option to GlslangToSpirv instead of considering modifying iomapper.

RuoyuAMD commented 4 years ago

@nitronoid https://github.com/KhronosGroup/glslang/pull/2224 I think maybe it can help you.

nitronoid commented 4 years ago

@Roy-AMD Yes! This looks perfect, thank you Roy.

johnkslang commented 4 years ago

Rather than making a change that would generate invalid SPIR-V, I wonder if using the UserSemantic decoration (or possibly the SPV_KHR_non_semantic_info) extension would work.

nitronoid commented 4 years ago

@johnkslang I would be happy with a solution utilising UserSemantic as that would keep the information contained within decorations. How would you propose encoding the "autogenerated" nature of the buffer's binding?

johnkslang commented 4 years ago

Maybe something like "HLL_default_value". (HLL meaning high-level language.)

nitronoid commented 4 years ago

That seems reasonable to me, although perhaps some indication that it was generated by glslang would be useful? Not a sticking point though, I'm happy with this as long as it can be relied upon.

Thank you!

johnkslang commented 4 years ago

The generator number at the top of the module says glslang generated it.

johnkslang commented 4 years ago

Also, other front ends should also be able to generate it.

nitronoid commented 4 years ago

The generator number at the top of the module says glslang generated it.

Ah of course. This would solve our issue, so I'm happy with it.

Thanks again