[dxil] Proposal to add new debug printf dxil op #324
Conversation
There was a problem hiding this comment.
Some comments inline. One general point - please format to 80-columns or so, it makes it much easier for reviewers to read.
I don't think we can design this feature in isolation. If the intention is to have the d3d debug layer, or PIX, somehow capture printf's then we need to make sure that any design we have here will meet the needs of these users of it.
I think we also need to make sure we're really comfortable with embedding the strings into individual compiled shaders. I'm concerned that this will lead to excessively large collections of shaders, once the feature is used in non-trivial scenarios.
We probably need to get alignment on direction on these previous two issues before proceeding further with the spec.
proposals/NNNN-debug-printf.md
Outdated
| const string str0= "str0"; | ||
| string str1 = "str1"; |
There was a problem hiding this comment.
It looks like this proposal is adding a new data type to HLSL, string? Or does this exist already? Are the semantics of this well understood? Can they be passed to / from functions, for example? How do they interact with any other existing builtin functions / operators?
There was a problem hiding this comment.
string is part of current semantics, for example.
const string first = "node"; [Shader(first)]
it can be used with the intrinsic which accept strings. but from my experimentation, it can not be used in function argument.
There was a problem hiding this comment.
It looks like there are restrictions on where strings can appear.
printf(str0);
This example seems to be passing a string as a function argument, is this going to be supported now?
printf("Variables are: %d %d %.2f", 1u, 2u, 1.5f);
This seems to have a string literal being passed as a function argument.
There was a problem hiding this comment.
The function mentioned is user defined function, but it seem instrinsic functions can use string variable.
There was a problem hiding this comment.
We don't want to be in a situation where built-in functions behave in a fundamentally different way to user-defined functions. Where this has been done in the past, this has led to inconsistent behavior with things like overload resolution. This is something we've been actively trying to address in the implementation of HLSL in Clang, and so any new features we add we want to be sure to add in a way that is compatible with that goal.
There was a problem hiding this comment.
The string seems touching some history baggage from clang. Now removing the string variables from this hlsl example.
proposals/NNNN-debug-printf.md
Outdated
|
|
||
| 2. The format string input to the dx.hl.op..void, could be llvm constant expression, we need to retrieve global variable from the constant expression. | ||
|
|
||
| 3. The validation for the dxil overloading should be handled properly. Because of printf variable arguments, there is no definite function type can be validated. |
There was a problem hiding this comment.
I'm not sure what "properly" means here. Is there something specific that needs to be validated?
There was a problem hiding this comment.
the printf is variable arguments, and first argument "format string" is a pointer to a string of different size. the function overload argument checking should be ignored.
| void main() { | ||
| printf(str0); | ||
| printf(str1); | ||
| printf("Variables are: %d %d %.2f", 1u, 2u, 1.5f); |
There was a problem hiding this comment.
If we decide to go further with this, we'll need to specify exactly how these format strings are interpreted.
There was a problem hiding this comment.
The format specifier generally follow the c/c++ format specifiers as here, https://www.geeksforgeeks.org/format-specifiers-in-c/,
Though how the final string representation after printf output is implementation of underlying d3d driver dependent.
dxc does not valiate format specifier to the c/c++ format speicifer standard, or the matching relation between
format specifier and argument. If the number and type don't match, they will produce undefined result from
underlying driver or debug layer
There was a problem hiding this comment.
I don't think we want to build a feature that DXC doesn't validate and where all behavior is undefined.
There was a problem hiding this comment.
this is part of design has been changed.
|
The updated rendered printf proposal md |
This is a counterpart proposal with similar debug printf feature from the spirv https://github.com/KhronosGroup/SPIRV-Registry/blob/main/nonsemantic/NonSemantic.DebugPrintf.asciidoc. The format string of printf is a const string variable, like OpString in the spirv.
proposals/NNNN-debug-printf.md
Outdated
| ## Detailed design | ||
|
|
||
| 1. The printf dxil op will be non-semantic, it does not affect final hlsl code/algorithm. | ||
| Non-semantic dxil op code can be counted down from 0xffff, or 0xffffffff, it will give a hint to the client api |
There was a problem hiding this comment.
We don’t have any notion of “non-semantic” operations in DXIL today, so this is a new concept. Also DIXL is not modifiable by the client API, which seems like a problem for this design.
There was a problem hiding this comment.
i change the wording a little , from non-semantic to the debug purpose dxil
There was a problem hiding this comment.
Changing the words doesn't solve the problem. We don't have a notion of instructions or operations that the client API can remove or that drivers can just ignore.
There was a problem hiding this comment.
okay, i will remove this part of the design.
This is a counterpart proposal with similar debug printf feature from the spirv https://github.com/KhronosGroup/SPIRV-Registry/blob/main/nonsemantic/NonSemantic.DebugPrintf.asciidoc. The format string of printf is a const string variable, like OpString in the spirv.