- How do we intend to position this? Is this intended as a debug tool that someone could enable with e.g. VKD3D_CONFIG? Or should it be a compilation option or even a separate API? How do we want this to relate to e.g. IDirect3DShaderValidator9? This has consequences for some of the other choices we might make, of course.
My intention is to have this essentially as a debug and documentation tool. As the `assert()` implies, ideally in no case an IR error should ever be detected, because it is assumed that the frontend already generates correct code, or fails if some error in its own input is detected. The problem is that a number of different IR frontends and backends are either being developed or planned, and the IR itself is relatively complicated, which might bring us in a situation in which even for the developers it's not obvious which are the IR rules, i.e., when an IR program is syntactically correct or not. This can lead to subtle bugs if the frontend and backend used for a given translation disagree on what is allowed and what is not in the IR.
So the first reason to have an IR validator is to bless it as a source of truth for what is correct IR and what is not, i.e., consider it the documentation for the IR, or at least for its syntax.
The second reason is that you can actually run it on each shader translation. If it flags an error, it means that your quest to find a bug might be much shorter than otherwise, because you know that at least a mistake is in the frontend and you also have some coordinates to find it.
Since under no case this validator should produce an error, there is little sense to expose it to the library client, with whatever API. If the validator flags something, it is a developer's job to pick it up, not a user's.
- In general, assert() seems like a mistake to me. In NDEBUG builds it does nothing, while on the other hand aborting usually isn't the most useful thing to do either. We could get a backtrace, but that'll just point to the validator instead of the place that introduced the issue.
I think we already discussed this in the past, but I disagree: I find that using `assert()` is useful to document the assumptions of some piece of code. If those assumptions end up being violated, there are two sensible behaviors: 1) do nothing, hoping for the best; or 2) make the world explode, so that a developer is more likely to check why something that should have never happened really happened. You sound like you're from camp 1, so you should probably just use `NDEBUG` (and notice that my code still gives some potentially useful information). But camp 2 makes sense to: it trades some short terms crashes for the ability to spot a bug before you have to spend a lot of time on debugging some less obvious symptoms. At any rate, defining or not `NDEBUG` makes it easy for everybody to choose and possibly change their own camp.
As I already mentioned, I admit that this case is a bit more special, so I can see some more compelling reason than usual to remove the `assert()`.
- This ties a bit into the question of positioning, but note that the compile functions generally (should) do a scan first. On the other hand, we might want to revalidate after e.g. vkd3d_shader_normalise() as well.
Yeah, that makes sense.
- I think we want to use the existing location information in the validator instead of reinventing it, and we should probably just use the message context from struct vkd3d_shader_parser for outputting messages.
Right, I don't know that in detail, but I will study it.
- Do we want to validate against the limits of the device or shader model here? E.g., the number of available constant registers or input/output registers depends on the shader model. Some features like stencil export require extension when targetting e.g. SPIR-V.
Yeah, I would say that most of these checks sound sensible. The idea is that the backend should be able to rely on the validator to be sure of what its input IR looks like. So it makes sense to have more or less validation strictness depending on what the backend can handle. Also, it would make sense to have a stricter check after normalisation (i.e., check that the IR post-normalisation is indeed normalised).
Hopefully all of this can be done is such a way to leave the validator as easy to read as possible, so that it's an effective documentation useful for the backend programmer.