-
Notifications
You must be signed in to change notification settings - Fork 36
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
Rework hlsl-vector-type into two specs #361
Conversation
Since we'll be creating a separate DXIL spec to document native vectors in DXIL, this spec will be a little more constrined to deal with HLSL long vectors. This commit is to isolate the meaningful content changes that come later
Make md lint happy, eliminate Load/StoreN, revise some wording
This splits the spec into two. dxil-vectors concerns the addition of vectors to DXIL only. hlsl-long-vector-type relates to the addition of long vectors in the HLSL language and also for select DXIL intrinsics. Throughout, this adds additional details concerning testing and support. It makes a few alterations to the originally proposed behavior particularly concerning the loading and storing of long vectors whether from/to raw buffers or groupshared variables. The latter intrinsics were dropped entirely in favor of existing assignment operations being lowered to appropriate operations. Long vectors are allowed in structs and non-entry function signatures and disallowed in shader signatures, cbuffers/tbuffers, and as elements of non-raw buffers. Note that the use of 6.9 is a placeholder for the release vehicle for this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general high-level points:
What gates this feature in HLSL? Is it part of a future HLSL language version? Just Shader Model 6.9 (which would seem odd considering this is basically a language change)? I don't see why we can't map long vectors to legacy scalarized shader models, with some requiring a bit more work, such as native vector load/store DXIL operations, vector reductions (dot) to expansions, and so on, but these are still mappable.
If not by language version, how do we enforce the limitations? Globally by shader model? Typically, shader model limitations are applied according to what's used, not what's present, so do we plan to do the same here (and how?), or use the shader model to change the language accepted everywhere, which is odd?
This doesn't address targeting SPIR-V from HLSL at all.
I still don't see how we will be able to support specialization constant sized vectors in HLSL or any equivalent feature in DXIL using this approach.
Many of the DXIL details I believe belong only in the DXIL spec. Some testing details in the DXIL spec I think belong only in the HLSL spec.
The limits we define should not be the temporary ones for implementation purposes, which could be discussed in some other section or doc perhaps. Limits on intrinsics with native vector DXIL overloads and limiting use in cbuffer/tbuffer could fall under this category.
No mention about vector and matrix swizzles lengths deliberately not being extended to allow long vector results.
I have some concern about 64-bit component types and in particular double
in long vectors, since there are a number of special cases for these. They might need additional testing scenarios, or to be added to temporary exclusions for preview.
This is definitely a discussion we should explore more. From a purity perspective my gut is that long vectors is a language feature for HLSL 202x. The downside to that is that anyone using SM 6.9 features for long vectors will need to update their codebase to HLSL 202x. This might cause us to reconsider the scoping of 202x to support calling it "done" sooner than originally planned. If we aren't okay with this feature requiring updating to 202x, it probably ins't the worst thing to make it available in older language modes, but we may want to consider making it an opt-in feature. I haven't thought about this extensively but this is definitely something we should discuss.
I don't think we should be thinking about specialization constants as part of this feature. HLSL does not have support for specialization constants. There is a Vulkan extension, but the Vulkan extension is not core to the language and it is not really the direction that the language-based solution would take. I realize that this feedback may seem counter to other feedback I've provided about needing a comprehensive plan for a feature. The distinction I see here is that designing specialization constant support for HLSL and fixing the existing language features that currently don't support specialization constants and should (like a bunch of the attributes, and operations where we require immediate values), is a much larger feature and will require significant investment and planning. Since we don't have a plan for how to support specialization constants in HLSL as a proper language feature, we shouldn't use that to roadblock or derail the design of this feature. If we make a mistake here and we need to adjust or redesign things when we add specialization constants in the future, we can cross that road when we get to it. We are all human, making design mistakes is something that is going to happen, we can't let the possibility of a mistake prevent us from making forward progress. |
The biggest changes are removing most references to scalarized implementation of certain intrinsics. This has the effect of removing any hard dependencies between the specs. This further strengthens my opinion that the specs should be divided along feature lines rather than the DXIL/language barrier. A lot of rewording and specifics added where vague statements were before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks all! I've responded inline and a commit is forthcoming.
Move asXXX interinsics to the approved list. Finish reworking validation errors and testing in long vectors spec. Simplify some listing of allowed locations given that some of them fall under entry function parameters by nature. I left work graphs as explicit since their parameters are not directly user-defined structs, but templates.
Anupama had some feedback on the description of where long vectors can be used. This attempts to add language that is more useful. Removed the mask param and made the load function independent per discussions with Tex and others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the spec proposal.
clarify range of long vectors Add text that constant integer expression for length requirement is maintained explicitly mention allowing local function scoped long vectors Elaborate on vector initialization from shorter vectors and initiailziation lists. Add potential SPIR-V solutions to issues. Explicitly state that native vectors can be dynamically accessed. Clean up language about intrinsics allowing scalars and vectors.
missed a couple
This splits the spec into two. dxil-vectors concerns the addition of vectors to DXIL only. hlsl-long-vector-type relates to the addition of long vectors in the HLSL language and also for select DXIL intrinsics.
Throughout, this adds additional details concerning testing and support. It makes a few alterations to the originally proposed behavior particularly concerning the loading and storing of long vectors whether from/to raw buffers or groupshared variables. The latter intrinsics were dropped entirely in favor of existing assignment operations being lowered to appropriate operations.
Long vectors are allowed in structs and non-entry function signatures and disallowed in shader signatures, cbuffers/tbuffers, and as elements of non-raw buffers.
Note that the use of 6.9 is a placeholder for the release vehicle for this feature.