[0002] Detail attributes that will replace HLSL annotations #534
Conversation
This adds language about removing HLSL annotation syntax as well as some proposed specification language. It also details the specific transformations of HLSL annotation attributes to general attributes.
There was a problem hiding this comment.
Overall I'm happy with this, unifying a bit how we write attributes across vk/hlsl.
I think we'd need to explicit a bit more the behavior changes between the APIs like how semantic indexing changes (same way you explicitly sais how register/index means binding/descriptor in the VK workd).
But overall, in the right direction!
| general behavior described here. | ||
|
|
||
| An empty attribute specifier has no effect. The order in which attributes | ||
| applied to the same source construct are written shall not be significant. When |
There was a problem hiding this comment.
So this means attribute overriding/duplication/conflict will be determined by each attribute definition?
Like if I did uint field [[hlsl::system_value(something), hlsl::system_value(something_else)]]
| In Vulkan, the first value maps as the descriptor index, and the second maps as | ||
| the descriptor set index. | ||
|
|
There was a problem hiding this comment.
Shall descriptor index be replaced with binding index? Since in SPIR-V those are Binding and DescriptorSet
|
|
||
| The following new attributes are introduced to replace HLSL annotations. | ||
|
|
||
| #### hlsl::user_value(string[, int=0]) |
There was a problem hiding this comment.
In SPIR-V, we currently ignore the user semantics. We assign a location based on the order in which variables are declared, or using the vk::location attribute. There have also been requests to allow the users to specify the component for the input as well as the location.
This does does not really work for SPIR-V since we have no good way to turn the string into a location index. The only thing we do with the the user semantic is to keep it around for reflection information.
I don't know if we want to try to come up with something like hlsl::binding that would be well defined for both DX and SPIR-V. Then we could deprecate vk::location, and close issues for the component.
Maybe something like hlsl::user_value(string[, int=0, int=0]), with the statement: "The interpretation of this attribute is
defined by the target runtime's binding model." Then we can define DX and Vulkan behaviour separately.
@Keenuts any more thoughts?
There was a problem hiding this comment.
I've read this more as a 'rewrite' of the attribute form with a new syntax, but also assumed we'd need to ignore this in SPIR-V since Location index is independant from the user semantic.
I think we could keep the split we have today and either use the parameter index ignoring the content, or ass vk-specific attributes for component/location.
void main([[hlsl_user_value("MYSEMANTIC"), vk::location(0), vk::component(1)]] float a)
| argument to the attribute is a string which can contain any valid C-string. The | ||
| second optional value is an index. | ||
|
|
||
| #### hlsl::system_value(enum[, int=0]) |
There was a problem hiding this comment.
@Keenuts Which system values have an index, and how do we handle those for SPIR-V in DXC?
There was a problem hiding this comment.
It depends on the system value:
Compute related ones have no index, same for semantic like POINT_SIZE. Those are builtins. But for SV_TARGET, which has an index, we emit a Location decoration.
llvm-beanz
changed the title
llvm-beanz
changed the title
| `c<row>.<column_letter>` map to the new attribute as `hlsl::packoffset(<row>, | ||
| <column_index>)`, where `<column_index>` maps as in the table below. | ||
|
|
||
| | column_letter | column_index | |
There was a problem hiding this comment.
considering payload_access allows identifiers, I'm curious why we are changing to integers here? The previous use of letters is fairly arbitrary and not any easier/harder to understand than integers, but the reverse holds, and this would be asking our users to get used to a new way of writing something
There was a problem hiding this comment.
My reasoning for getting rid of the prefix character c is that it can only ever be c which has meaning in DirectX (it's a CBV resource), but has no meaning in other binding models.
In terms of the column index. I can see an argument for it remaining a letter. I struggle a bit with it because it is effectively a sub-index to 32-bit offsets, which actually isn't valid for all types. For example you can't use an offset of y today for a 64-bit data member so your offsets available are only x and z, which just feels weird. Not that offsets 0 and 2 is really entirely better.
There was a problem hiding this comment.
I guess the other advantage that integers would have is that they can be integer constant expressions not just static values. So using integers would enable someone to do something like:
template <typename T, int X>
constexpr int toOffset() {
constexpr int Val = (sizeof(T) / 4) * X;
static_assert(Val <= 3);
return Val
}
...
double V [[hlsl::packoffset(0, toOffset<double,2>())]];
We could also consider supporting either integers or identifiers.
I'd be curious for @tex3d's thoughts here too.
There was a problem hiding this comment.
My reasoning for getting rid of the prefix character
c
Yeah I wasn't concerned about dropping the c. More so that we'd be moving somewhat arbitrarily from hlsl::binding(0,y) to hlsl::binding(0,2).
I think it's not really an issue since we are also rewriting the rest of the attribute but its at least worth considering given the tools and guides written around the notion that the second value is a letter.
I personally thing integer is simpler/clearer and thing from a clean slate that its the right decision, I just want to make sure we aren't negatively impacting users
| > Note: an example here would be optimization hint attributes which an | ||
| > implementation is allowed to ignore without diagnosing. | ||
|
|
||
| Each attribute may specify specific behavior for parsing attribute arguments. |
There was a problem hiding this comment.
After reading the full PR, I'm still not really sure what this is trying to say.
Are we saying that "special attributes may treat arguments specially"? Because if so, won't every attribution treat its arguments specially? If they weren't special, then the args would just be dropped?
Or is there some special attribute form I'm missing somewhere?
There was a problem hiding this comment.
Not all attributes have arguments, and of the ones that do, not all of them have special parsing logic. For example the numthreads attribute takes 3 integers, and they're just required to be integer constant expressions.
By contrast, the RootSignature attribute has complex special parsing rules (although maybe if you squint you could just say it is a string).
In Clang we're also using the availability attribute, which has even more special parsing logic because it supports named arguments (https://clang.llvm.org/docs/AttributeReference.html#availability). We're currently using the availability attribute in the GNU form (e.g. __availability__((shadermodel, introduced=6.3))) because of a bug in Clang's parsing of the C++11 version, but we should fix that so that it is more like [[clang::availability(shadermodel, introduced=6.3)]].
I'm open for suggestions on how to make this wording more clear.
There was a problem hiding this comment.
Ah thanks, that helps me understand.
Just kind of winging it here:
Attributes that require arguments may also require custom parsing rules, such as the availability attribute. Attributes without arguments and attributes with trivial parsing such as integer literals do not need custom parsing and may rely on the general behavior described here.
This adds language about removing HLSL annotation syntax as well as some proposed specification language. It also details the specific transformations of HLSL annotation attributes to general attributes.
Fixes #527