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

Add 'aliases' field to show symbol aliasing relationship #138

Closed
wants to merge 1 commit into from

Conversation

antiagainst
Copy link
Contributor

It's common to pull symbols first introduced in extensions to the
core spec when SPIR-V version bumps. For such cases, the underlying
value remains the same; it's just that we introduce a new symbol
(without extension suffix) for it. Thus far we have been creating
a entire new case for such cases incorporated into core. It's
cluttering the grammar and complicates downstream consumers
because downstream consumers need to understand the relationship.
It's also quite confusing and error-prone: when introducing a new
symbol, it can have different fields than the original one, either
meaning subtle differences or caused by oversight.

Instead, this commit introduces an 'aliases' field to properly
show the relationship between symbols. Other fields describing
the value, including 'version', 'extensions', etc., are cleaned
up to make sure the canonical symbol collects all the information
from duplicated cases. Now if a value has both 'version' and
'extensions', it means the value is available either starting
from the given version, or with any of the extensions.

It's common to pull symbols first introduced in extensions to the
core spec when SPIR-V version bumps. For such cases, the underlying
value remains the same; it's just that we introduce a new symbol
(without extension suffix) for it. Thus far we have been creating
a entire new case for such cases incorporated into core. It's
cluttering the grammar and complicates downstream consumers
because downstream consumers need to understand the relationship.
It's also quite confusing and error-prone: when introducing a new
symbol, it can have different fields than the original one, either
meaning subtle differences or caused by oversight.

Instead, this commit introduces an 'aliases' field to properly
show the relationship between symbols. Other fields describing
the value, including 'version', 'extensions', etc., are cleaned
up to make sure the canonical symbol collects all the information
from duplicated cases. Now if a value has both 'version' and
'extensions', it means the value is available either starting
from the given version, or with any of the extensions.
@johnkslang
Copy link
Member

Thanks Lei. The aliases addition look good, but I think it's too early for removing the differences (union-ing the content) between the duplicated entries.

Potentially, I believe, some differences are intentional between the duplicated entries, giving them value (e.g., the suffix comes from extension X, while no suffix comes from core).

Copying in the multi-step prescription from the closed PR:

  1. alias field for just what you care about, nothing else added or removed
  2. alias field for all things that are aliased
  3. SPIR-V specification generation tooling updated to understand it
  4. SPIRV-Tools and other things(?) updated to understand it
  5. redundant existing way of duplicating entries is eliminated [edit: and union the content]

The point of the first one or two steps is that it can't break anything, because it's only a new field being added.

Adding new entries with existing field names will change the way existing tools behave. I was considering (but failed to write explicitly in those steps) that such union-ing was part of the last step of removing redundancies.

Can we have a PR that just adds a new field, to give tooling a chance to catch up, before changing existing fields in existing entries?

@antiagainst
Copy link
Contributor Author

antiagainst commented Jan 20, 2020

Based on my observation, the grammar is inconsistent as for how it handles such "duplicated" entries.

  • Some "duplicated" entries have exactly the same content except the symbol suffix, e.g., OpDecorateString vs OpDecorateStringGOOGLE.
  • Some core entries have more extensions than the entries for an extension, e.g., PhysicalStorageBufferAddresses vs PhysicalStorageBufferAddressesEXT.
  • Some core entries do not have any extensions while their extension counterpart has, e.g., MakeVisible vs MakeVisibleKHR.
  • Some extension entries have completely different extensions than another one, e.g., ShaderViewportIndexLayerEXT vs ShaderViewportIndexLayerNV.

So IMO this relationship is already broken; therefore downstream tooling is already likely in a confused or subtly broken state that needs to be fixed.

I can certainly put up a PR to only strictly add aliases field. But itself solely I don't see how it can help to remedy the problem. We need to fix this issue anyway; be it a few commits later or now there is gonna to be a breaking change. If we just add the aliases field but do not unify the content into the canonical entry in hoping of letting tooling to "catch up", downstream tooling actually needs to throw in more logic to analyze the relationship (which one of the above three cases is it? From which entry should I get what information?) and handle them. Note that this logic will be a one-off effort that will be thrown away later when we introduce the real breaking change: tooling will be forced to change again. So I don't quite see why that is preferrable. Hopefully this explains why I bundle adding the aliases field together with unifying the content into the canonical entries. But I might miss something; happy to hear what you think. :)

@johnkslang
Copy link
Member

johnkslang commented Jan 20, 2020

Yes, it is logical, but I've been bit a few times recently with logical changes still unintentionally breaking consumers. There are some ad hoc things going on, and having the specification get printed differently would also deserve doing a diff and a change log entry explaining the differences.

The solid thing to do is first add the new information, give tools a window of time to react/code to that, and then switch over to the final correct method. This will decouple, e.g., specification publication schedule from the schedule of this series of PRs.

[edit: I'm not saying the one-off code has to be written, that can still be skipped.]

This will result in a one-time change in behavior/specification for this change instead of two times.

@Naghasan
Copy link
Member

closing in favour of #447

@Naghasan Naghasan closed this Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants