Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
cmarcelo opened this issue May 21, 2019 · 7 comments

Comments

@cmarcelo
Copy link
Contributor

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
Copy link
Member

We've agreed to evolve this so that:

  • the first appearance of a token specifies everything about it
  • the spec. itself merges this and all its aliases into a single box/row (as already done for instructions, but not yet done for other classes of enumerants)
  • subsequent aliases of the first appearance list only the name and number, neither replicating nor contradicting any of the other fields

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

@johnkslang johnkslang changed the title Possibly wrong extensions and version for the aliased operands? Convention and intent for grammar for aliases. (Was: Possibly wrong extensions and version for the aliased operands?) May 21, 2019
@cmarcelo
Copy link
Contributor Author

cmarcelo commented Jun 8, 2019

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
Copy link
Member

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
Copy link
Contributor

dneto0 commented Jun 10, 2019

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
Copy link
Member

Also see #108 for any loose ends in documentation.

@spencer-lunarg
Copy link
Contributor

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 )

@spencer-lunarg
Copy link
Contributor

@Naghasan has merged in #447 which might solve this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants