KhronosGroup / SPIRV-Headers

SPIRV-Headers
Other
258 stars 249 forks source link

Convention and intent for grammar for aliases. (Was: Possibly wrong `extensions` and `version` for the aliased operands?) #109

Open cmarcelo opened 5 years ago

cmarcelo commented 5 years ago

Should GOOGLE aliases be tagged with 1.4? (I'm guessing no) Should the non-GOOGLE variants have empty "extensions" field? (I'm guessing yes)

I'd expect decorate string and hlsl-related annotations to follow the same convention, but they disagree in both questions.

Decorate string (with params omitted):

    {
      "opname" : "OpDecorateString",
      "opcode" : 5632,
      "extensions" : [ "SPV_GOOGLE_decorate_string", "SPV_GOOGLE_hlsl_functionality1" ],
      "version" : "1.4"
    },
    {
      "opname" : "OpDecorateStringGOOGLE",
      "opcode" : 5632,
      "extensions" : [ "SPV_GOOGLE_decorate_string", "SPV_GOOGLE_hlsl_functionality1" ],
      "version" : "1.4"
    },
    {
      "opname" : "OpMemberDecorateString",
      "opcode" : 5633,
      "extensions" : [ "SPV_GOOGLE_decorate_string", "SPV_GOOGLE_hlsl_functionality1" ],
      "version" : "1.4"
    },
    {
      "opname" : "OpMemberDecorateStringGOOGLE",
      "opcode" : 5633,
      "extensions" : [ "SPV_GOOGLE_decorate_string", "SPV_GOOGLE_hlsl_functionality1" ],
      "version" : "1.4"
    },

Hlsl-related annotations (with params omitted):

        {
          "enumerant" : "CounterBuffer",
          "value" : 5634,
          "version" : "1.4"
        },
        {
          "enumerant" : "HlslCounterBufferGOOGLE",
          "value" : 5634,
          "extensions" : [ "SPV_GOOGLE_hlsl_functionality1" ],
          "version" : "None"
        },
        {
          "enumerant" : "UserSemantic",
          "value" : 5635,
          "version" : "1.4"
        },
        {
          "enumerant" : "HlslSemanticGOOGLE",
          "value" : 5635,
          "extensions" : [ "SPV_GOOGLE_hlsl_functionality1" ],
          "version" : "None"
        },
johnkslang commented 5 years ago

We've agreed to evolve this so that:

There was some tooling that wasn't quite ready for this yet, so the JSON file is not yet sanitized in this way.

cmarcelo commented 5 years ago

So, taking the examples above and modify them to follow the new rules. The decorate string ones would only omit fields in aliases, without changes:

    {
      "opname" : "OpDecorateString",
      "opcode" : 5632,
      "extensions" : [ "SPV_GOOGLE_decorate_string", "SPV_GOOGLE_hlsl_functionality1" ],
      "version" : "1.4"
    },
    {
      "opname" : "OpDecorateStringGOOGLE",
      "opcode" : 5632,
    },
    {
      "opname" : "OpMemberDecorateString",
      "opcode" : 5633,
      "extensions" : [ "SPV_GOOGLE_decorate_string", "SPV_GOOGLE_hlsl_functionality1" ],
      "version" : "1.4"
    },
    {
      "opname" : "OpMemberDecorateStringGOOGLE",
      "opcode" : 5633,
    },

And the HLSL related ones:

        {
          "enumerant" : "CounterBuffer",
          "value" : 5634,
          "extensions" : [ "SPV_GOOGLE_hlsl_functionality1" ],
          "version" : "1.4"
        },
        {
          "enumerant" : "HlslCounterBufferGOOGLE",
          "value" : 5634,
        },
        {
          "enumerant" : "UserSemantic",
          "value" : 5635,
          "extensions" : [ "SPV_GOOGLE_hlsl_functionality1" ],
          "version" : "1.4"
        },
        {
          "enumerant" : "HlslSemanticGOOGLE",
          "value" : 5635,
        },

Is that correct?

If so, why not just add a "aliases" field in the main opcode, and have a single entry per opcode? Room for adding more per-alias data?

johnkslang commented 5 years ago

My understanding is there are still some consumers that want both sets of complete information. I like the idea of adding an aliases field. These questions need to be answered both for instruction enumerants and for non-instruction enumerants, which generally are treated a bit differently.

We need to evolve this while keeping the spec., SPIRV-Tools, and likely other consumers in mind.

(It seems you just cared about the instructions(?), as this style of aliasing has been present for some time in other enumerants; it was not novel, it was following the precedent already set.)

dneto0 commented 5 years ago

I agree it's a good direction to ensure consistency for each token. We haven't implemented the update to SPIRV-Tools to understand the new form. I filed an issue (see above) to do that.

johnkslang commented 4 years ago

Also see #108 for any loose ends in documentation.

spencer-lunarg commented 7 months ago

So this "the first item is the truth" isn't followed here

 {
      "opname" : "OpTypeAccelerationStructureNV",
      "class"  : "Reserved",
      "opcode" : 5341,
      "operands" : [
        { "kind" : "IdResult" }
      ],
      "capabilities" : [ "RayTracingNV" , "RayTracingKHR", "RayQueryKHR" ],
      "extensions" : [ "SPV_NV_ray_tracing" , "SPV_KHR_ray_tracing", "SPV_KHR_ray_query" ],
      "version" : "None"
    },
    {
      "opname" : "OpTypeAccelerationStructureKHR",
      "class"  : "Reserved",
      "opcode" : 5341,
      "operands" : [
        { "kind" : "IdResult" }
      ],
      "capabilities" : [ "RayTracingNV" , "RayTracingKHR", "RayQueryKHR" ],
      "extensions" : [ "SPV_NV_ray_tracing" , "SPV_KHR_ray_tracing", "SPV_KHR_ray_query" ],
      "version" : "None"
    },

I guess personally, instead of worrying about ordering, I would also prefer something like a "alias" : "OpTypeAccelerationStructureKHR"

(cc @bashbaug )