vkd3d-shader/dxil: Implement DX instruction StoreOutput.
Merge request reports
Activity
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
It might be that this ship has already sailed, since this is probably something preceding this MR, but I feel a bit awkward about how errors are silently ignored in a number of code paths (see for example
sm6_parser_emit_dx_store_output()
, but it's not the only place). I understand that we don't want to be overly zealous with rejecting invalid shaders, because it might be that even if our translation isn't perfect the translated shader is still good enough so that, say, a certain game is playable.However, at least from the point of view of vkd3d-shader (which doesn't know how that shader is going to be used), this doesn't really feel correct. Maybe the average game doesn't really care if a shader is broken, since the bad case scenario is that the player just decides that the game isn't fun and proceeds doing something else. But other applications might be more demanding. For example, if you allow me a rather extreme example, a medical application might be much less failure-tolerant, to the point of preferring declaring a malfunction rather than plotting wrong medical data. I'm not aware of any such usage, currently, for vkd3d-shader, but I consider this an example to infer that the decision of still using a shader even if it's known to be incorrectly translated should not belong to vkd3d-shader. It makes sense for vkd3d-shader to still report an incorrectly translated shader as a best guess, but the error code should declare what's happening, so that the client can choose meaningfully.
What is the feeling about this issue?
Why not always returning the error? What is the advantage of doing some more parsing, if the only information we're going to return is just a generic failure error, without additional diagnostics?
Well, we should be providing the additional diagnostics. But generally, for much the same reasons that e.g. the HLSL compiler doesn't abort at the first sign of trouble. There may be multiple unrelated issues in the same shader, and knowing about subsequent issues sooner rather than later is generally helpful. At the very least because it might give someone interested in working on an application a better sense of the amount of effort required to make it work.
It might be that this ship has already sailed, since this is probably something preceding this MR, but I feel a bit awkward about how errors are silently ignored in a number of code paths (see for example
sm6_parser_emit_dx_store_output()
, but it's not the only place). I understand that we don't want to be overly zealous with rejecting invalid shaders, because it might be that even if our translation isn't perfect the translated shader is still good enough so that, say, a certain game is playable.No, I think you're right about that; the default behaviour should be to reject invalid shaders. We may want to be able to relax that in some cases, but we could introduce compile options for that, perhaps enabled by VKD3D_CONFIG flags on the vkd3d side.
- Resolved by Giovanni Mascellani
2246 2471 FIXME("Unhandled dxil instruction %u.\n", record->code); 2247 2472 return VKD3D_ERROR_INVALID_SHADER; 2248 2473 } 2474 /* Allocation failure. */ That's not necessarily the case, is it? Any call to
vkd3d_shader_parser_error()
, such as indxil_record_validate_operand_min_count()
, could trigger this failure.More in general, I think we there is some confusion between whether to use the parser
failed
bit vs using the return value. We could use the same convention as the HLSL compiler:failed
is set as soon as an error is found, and it should always set viavkd3d_shader_parser_error()
so that a diagnostic is always available; an error return value should only be used when an error was found and parsing must halt immediately.In this scheme you should never look at
failed
to decide what to return, because you don't know iffailed
was set because of an error that still allows parsing or not. You have to genuinely propagate the return code at each level.It is useful to have memory allocation functions that also set the
failed
bit, like the HLSL compiler.changed this line in version 6 of the diff
- Resolved by Giovanni Mascellani