vkd3d-shader/dxil: Implement DX instruction LoadInput.
Merge request reports
Activity
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
The code seems fine to me.
A general design question though: it seems that you could avoid introducing a new register type simply by creating "regular" temporary registers instead of SSA ones. An SSA register has the additional property that you cannot write it after the first time, essentially, but that doesn't prevent you from using a temporary and just writing it once. Would there be a fundamental problem with that approach? If not, then why did you decide to introduce a new type?
And for the record I don't mean to say that I don't want SSA registers (and I see a couple of positive reasons to have them), I would just like to know what's the reasoning. Even better would be to have that reasoning documented in the commit message introducing SSA registers (yes, commit messages might not be ideal for discoverability in the long term, but we currently don't have a real place to store developer documentation and that's better than nothing).
added 1 commit
- 6dc18aa3 - vkd3d-shader/dxil: Implement DX instruction LoadInput.
added 5 commits
- f8e88452 - vkd3d-shader/spirv: Introduce a Static Single Assignment register type.
- bcde44a7 - vkd3d-shader/spirv: Build undefined values once.
- 0d8bd4c1 - vkd3d-shader/dxbc: Load input signatures also from ISG1 chunks.
- 0075bcbe - vkd3d-shader/dxil: Declare shader inputs.
- 98d598d4 - vkd3d-shader/dxil: Implement DX instruction LoadInput.
Toggle commit listshader_component_type_size()
needs to be higher up for the next series. EDIT: this applies to the above push. I have nothing pending here.Edited by Conor McCarthyA general design question though: it seems that you could avoid introducing a new register type simply by creating "regular" temporary registers instead of SSA ones. An SSA register has the additional property that you cannot write it after the first time, essentially, but that doesn't prevent you from using a temporary and just writing it once. Would there be a fundamental problem with that approach? If not, then why did you decide to introduce a new type?
For the record, I have the same concern.
Both DXIL and SPIR-V use SSA, so using it in the IR is by far the simplest way to handle the values.
I don't think the second part of that statement necessarily follows from the first part.
Using temps would introduce the problem of selecting an unused temp, i.e. one whose value is no longer needed, and it becomes even more complex when dealing with
PHI
instructions.Why is that?
Also, temps are written and read with
OpStore
andOpLoad
, which SSA renders unnecessary. I'm not inclined to add a comment on this as I think the question won't arise when everything is upstream.How valuable is that? If it's significant, we may want to consider converting TPF to SSA form as well in the SPIR-V backend.
It does become a bit of an issue with upstreaming this though; in principle upstreaming dxil support isn't affected much by the current freeze, but that's predicated on not requiring significant changes to the existing code. Making significant changes to the shared IR or SPIR-V backend would likely need to wait until after the 1.9 release.
On revisiting this I think PHI instructions should disappear if temps are handled correctly, and it would eliminate some complications in the structuriser. On the other hand, optimisation of code generation by Vulkan drivers may be compromised when temps are used instead of SSA. We would need to test it to know for sure, but if it's ok, there's no guarantee we won't encounter another driver later which has issues.
How valuable is that? If it's significant, we may want to consider converting TPF to SSA form as well in the SPIR-V backend.
I have wondered that too. TPF's structured control flow probably makes it not especially complicated to do, even though it will need PHI instructions. If
OpStore
andOpLoad
are an issue, we should see performance improvements in, e.g. SotTR with this change. I'll try that unless anyone has a better idea.How valuable is that? If it's significant, we may want to consider converting TPF to SSA form as well in the SPIR-V backend.
It seems to me quite likely that translating sm4 to SSA would be more expensive than passing it to the driver (and letting the driver translate it to SSA internally, probably), but translating sm6 to temps would be more expensive than passing it through to the driver. I don't think we have any special knowledge that would let us convert sm4 to SSA more efficiently. Granted, maybe those differences aren't significant, but I hesitate to just dismiss them out of hand.
Also: if we are going to translate HLSL to vsir, and then perform optimizations, or raising passes, on that vsir, then it is distinctly easier to have those in SSA form, then convert the SSA form to registers once the actual instruction sequence has been finalized.
I haven't tried it, nor actually read Conor's code, but it doesn't seem likely that adding SSA is going to complexify the code that much. [Tracking def-use chains would, but I don't think we need that...]
How valuable is that? If it's significant, we may want to consider converting TPF to SSA form as well in the SPIR-V backend.
It seems to me quite likely that translating sm4 to SSA would be more expensive than passing it to the driver (and letting the driver translate it to SSA internally, probably), but translating sm6 to temps would be more expensive than passing it through to the driver.
To be clear, my expectation would be that these redundant OpStore and OpLoad instructions largely don't matter. I could very well be wrong about that, of course. And perhaps there are other reasons to avoid using temps here, but I haven't seen those.
So what I imagine we want to do, is to start with naively translating DXIL SSA values to vsir temp assignments. That should unblock upstreaming plenty of bits. We don't have control flow yet, we don't have phi instructions yet either, and it's not clear to me that there's a fundamental reason e.g. vsir phi instructions couldn't operate on vsir temps anyway. I think there are a few scenarios that could happen from there:
- It's fine.
- We introduce vsir SSA registers for some other reason, e.g. because the HLSL compiler wants to do optimisations in that form; DXIL/SPIR-V could then just take advantage of that.
- We encounter some issue that can't reasonably be resolved with vsir temps. We discuss it when we get there, instead of now in the abstract.
- We do some benchmarking/profiling and find that there are either advantages in compile time or run time to introducing a separate register type. We discuss it when we get there, with the hard data to back it up, instead of speculating about it now.
And perhaps this is also a good time to reflect on the broader upstreaming strategy. On a very broad level, the sequence that I would have hoped/expected to see would be something along these lines:
- Some shader_runner infrastructure to compile and run DXIL shaders.
- The most basic, straightforward implementation of the bits required to make a fairly minimal test pass. E.g., tests/hlsl/swizzles.shader_test.
- Basic, straightforward implementations of features required to make the rest of the tests pass.
- Features required by applications, but not covered by the tests. Writing tests for these as they're implemented.
- Optimisations and other complications.
Also, I'd like to stress this because it has come up before for other MRs, please don't pre-emptively add complications for issues that will only come up later; whether that's in a later patch in the same series or 400 patches later in the branch you're upstreaming from. Reviewers generally can't or don't want to look that far ahead, and it just ends up slowing the entire process down. Patches should make sense in isolation, at the point in time where they're introduced.
Just to explain my acceptance, I think too that the ideal flow would be to first convert SSA to temporary load and store, and introduce later SSA if there is an advantage. Dealing with loads and stores is something I expect downstream compilers to do pretty well (even the HLSL compiler does that, to a certain extent!), so I wouldn't stress too much over that. Phi nodes shouldn't be too much of a hassle either: just before emitting a jump you look at the phi nodes of the target block and convert them to assignments, unless I am missing something.
That said, I don't think it is unreasonable to go with SSA from the get-go, so I accept this MR and leave to Henri to do the actual software architecture part.
Also, I'd like to stress this because it has come up before for other MRs, please don't pre-emptively add complications for issues that will only come up later; whether that's in a later patch in the same series or 400 patches later in the branch you're upstreaming from. Reviewers generally can't or don't want to look that far ahead, and it just ends up slowing the entire process down. Patches should make sense in isolation, at the point in time where they're introduced.
I agree strongly with this in general. I find myself often asking people to do this. But—and I'm sure I'm missing some perspective here—I don't see it as being a problem in this specific case.
Prematurely adding complications is a problem, I think, when I (as a reviewer) can't see how they're going to be used, and when I feel that there might be a better, simpler option that will possibly preclude the more complex code from ever being necessary.
I don't see an issue with the first part; I have the barest of knowledge about sm6 but I can tell that if it has SSA nodes, then we're introducing a vsir SSA node to translate from sm6 SSA nodes and that it's going to be used to translate to spirv SSA nodes.
As for whether there's a better, simpler option—I have a hard time personally seeing temps as simpler. This is partly because the SSA register type is just not complicated to begin with, and I don't expect the sm6 -> vsir or vsir -> spirv translations thereof to be complicated either. But it's also because passes on vsir (both the ones I'm anticipating related to sm1/sm4 output, and the ones Conor is talking about) are going to be distinctly harder to do on temps than on SSA nodes. [Some of this is armchair analysis, some of this may be bias from having just written the code, but some of it is influenced by comparing copy-prop to our other HLSL passes. Copy-prop is more complex and harder to reason with in general, and I think roughly models how we'd have to do passes on non-SSA.]
(And, well, I'm more than a little concerned about compilation speed, and compiled shader size, which makes me think that we likely will SSA nodes in vsir at some point. But I don't have any data to back that up, so I'll let go of that concern for now...)
Once branched shaders are supported, assigning temps will require parsing the code graph to check all possible paths for temp usage. DXIL code graphs are spaghetti, and this is not a problem I am interested in solving.
We encounter some issue that can't reasonably be resolved with vsir temps.
vsir temps can't reasonably be assigned without a lot of work which doesn't need to be done.
SPIR-V converted from DXIL is already somewhat bloated because it does nearly everything with scalars. Adding all the access chains, loads, stores and some bitcasts will make the shaders much larger.
I see no cause for concern with
VKD3DSPR_SSA
. It's a new register type not used in TPF, the new code is not executed for TPF, and aside from a few checks for the new type it has no effect on existing code.added 5 commits
- a35f4590 - vkd3d-shader/spirv: Introduce a Static Single Assignment register type.
- 417fde8b - vkd3d-shader/spirv: Build undefined values once.
- 69e50092 - vkd3d-shader/dxbc: Load input signatures also from ISG1 chunks.
- 3c1762ac - vkd3d-shader/dxil: Declare shader inputs.
- 51f38089 - vkd3d-shader/dxil: Implement DX instruction LoadInput.
Toggle commit listI removed most of the code from
spirv_compiler_emit_load_ssa_reg()
as we don't need it yet.The sm6_rebase branch has some draft changes to shader runner. It introduces an
[enable]
directive where specifyingdxil
runs the test in SM 6. If this is missing the test is skipped for SM 6. Some tests can't be compiled with DXC. Currently it uses an environment variable to access DXC, but as was discussed some time ago, calling DXC's C++ library functions from C doesn't work. I'll revisit this to see if I can hack a way to match up the parameters.I tested an incrementing temp register index for each value. For a simple pass-through pixel shader, here is the SPIR-V function with SSA:
%4 = OpLabel %13 = OpLoad %v4float %gl_FragCoord %14 = OpCompositeExtract %float %13 3 %16 = OpFDiv %float %float_1 %14 %17 = OpCompositeInsert %v4float %16 %13 3 OpStore %v0 %17 %21 = OpInBoundsAccessChain %_ptr_Private_float %v0 %uint_0 %22 = OpLoad %float %21 %24 = OpInBoundsAccessChain %_ptr_Private_float %v0 %uint_1 %25 = OpLoad %float %24 %27 = OpInBoundsAccessChain %_ptr_Private_float %v0 %uint_2 %28 = OpLoad %float %27 %30 = OpInBoundsAccessChain %_ptr_Private_float %v0 %uint_3 %31 = OpLoad %float %30 %33 = OpInBoundsAccessChain %_ptr_Output_float %o0 %uint_0 OpStore %33 %22 %34 = OpInBoundsAccessChain %_ptr_Output_float %o0 %uint_1 OpStore %34 %25 %35 = OpInBoundsAccessChain %_ptr_Output_float %o0 %uint_2 OpStore %35 %28 %36 = OpInBoundsAccessChain %_ptr_Output_float %o0 %uint_3 OpStore %36 %31 OpReturn OpFunctionEnd
And with temps:
%4 = OpLabel %17 = OpLoad %v4float %gl_FragCoord %18 = OpCompositeExtract %float %17 3 %20 = OpFDiv %float %float_1 %18 %21 = OpCompositeInsert %v4float %20 %17 3 OpStore %v0 %21 %25 = OpInBoundsAccessChain %_ptr_Private_float %v0 %uint_0 %26 = OpLoad %float %25 %27 = OpInBoundsAccessChain %_ptr_Private_float %r0 %uint_0 OpStore %27 %26 %29 = OpInBoundsAccessChain %_ptr_Private_float %v0 %uint_1 %30 = OpLoad %float %29 %31 = OpInBoundsAccessChain %_ptr_Private_float %r1 %uint_0 OpStore %31 %30 %33 = OpInBoundsAccessChain %_ptr_Private_float %v0 %uint_2 %34 = OpLoad %float %33 %35 = OpInBoundsAccessChain %_ptr_Private_float %r2 %uint_0 OpStore %35 %34 %37 = OpInBoundsAccessChain %_ptr_Private_float %v0 %uint_3 %38 = OpLoad %float %37 %39 = OpInBoundsAccessChain %_ptr_Private_float %r3 %uint_0 OpStore %39 %38 %40 = OpInBoundsAccessChain %_ptr_Private_float %r0 %uint_0 %41 = OpLoad %float %40 %43 = OpInBoundsAccessChain %_ptr_Output_float %o0 %uint_0 OpStore %43 %41 %44 = OpInBoundsAccessChain %_ptr_Private_float %r1 %uint_0 %45 = OpLoad %float %44 %46 = OpInBoundsAccessChain %_ptr_Output_float %o0 %uint_1 OpStore %46 %45 %47 = OpInBoundsAccessChain %_ptr_Private_float %r2 %uint_0 %48 = OpLoad %float %47 %49 = OpInBoundsAccessChain %_ptr_Output_float %o0 %uint_2 OpStore %49 %48 %50 = OpInBoundsAccessChain %_ptr_Private_float %r3 %uint_0 %51 = OpLoad %float %50 %52 = OpInBoundsAccessChain %_ptr_Output_float %o0 %uint_3 OpStore %52 %51 OpReturn OpFunctionEnd
I can make it like this if necessary, but IMO using temps this way is an ugly hack which makes debugging SPIR-V traces harder. Once 1.9 is out, temps must be replaced with SSA because using them is a dead end. We should not attempt to implement a temp register allocator which works on DXIL control flow graphs.
Also, I'd like to stress this because it has come up before for other MRs, please don't pre-emptively add complications for issues that will only come up later; whether that's in a later patch in the same series or 400 patches later in the branch you're upstreaming from. Reviewers generally can't or don't want to look that far ahead, and it just ends up slowing the entire process down. Patches should make sense in isolation, at the point in time where they're introduced.
I agree strongly with this in general. I find myself often asking people to do this. But—and I'm sure I'm missing some perspective here—I don't see it as being a problem in this specific case.
Well, part of the context is at which point in the release process we are. We were about a week into the freeze when this was first submitted, and we're about a week from the release now. If this had been submitted in e.g. the first month or so after the 1.8 release, I would probably still have grumbled about the process and posted a comment along the lines of "please do better", but it may have ultimately gone in. As it is, I don't think so, and for the most part that's simply because the MR is touching things outside dxil.c that it doesn't strictly have to touch.
Prematurely adding complications is a problem, I think, when I (as a reviewer) can't see how they're going to be used, and when I feel that there might be a better, simpler option that will possibly preclude the more complex code from ever being necessary.
I don't see an issue with the first part; I have the barest of knowledge about sm6 but I can tell that if it has SSA nodes, then we're introducing a vsir SSA node to translate from sm6 SSA nodes and that it's going to be used to translate to spirv SSA nodes.
As for whether there's a better, simpler option—I have a hard time personally seeing temps as simpler.
Which parts of the system are you considering when you make that evaluation? Introducing SSA nodes is certainly simpler if you consider dxil.c in isolation, or even the dxil -> spirv path in isolation. That's (trivially) not true when you consider e.g. d3d_asm.c or spirv.c in isolation. Similarly, SSA nodes are extra work for someone adding e.g. a MSL or GLSL backend. What about transformation passes in ir.c? Are those potentially going to need to handle VKD3DSPR_SSA? We don't run any of the existing ones on dxil sources, but that doesn't make the consideration just go away.
It gets more complicated when you consider vkd3d-shader as a whole. We add some complexity in some places, but avoid some in others. We probably also eventually want SSA nodes for the HLSL compiler anyway. Is that worth it in the end? Well, probably.
What we're concerned about here though, is review/upstreaming complexity. And it's not even close there; SSA nodes introduce the evaluation above, as well as e.g. considerations about whether it's prudent to introduce these at this point in the release process, while for changes confined to dxil.c it would be only slight more complicated than "Well, we didn't support dxil before, so it's not terribly likely to make things worse."
This is partly because the SSA register type is just not complicated to begin with, and I don't expect the sm6 -> vsir or vsir -> spirv translations thereof to be complicated either. But it's also because passes on vsir (both the ones I'm anticipating related to sm1/sm4 output, and the ones Conor is talking about) are going to be distinctly harder to do on temps than on SSA nodes. [Some of this is armchair analysis, some of this may be bias from having just written the code, but some of it is influenced by comparing copy-prop to our other HLSL passes. Copy-prop is more complex and harder to reason with in general, and I think roughly models how we'd have to do passes on non-SSA.]
(And, well, I'm more than a little concerned about compilation speed, and compiled shader size, which makes me think that we likely will SSA nodes in vsir at some point. But I don't have any data to back that up, so I'll let go of that concern for now...)
Sure; I don't think anyone raised concerns about introducing SSA nodes per se. The issue is mostly about whether the rest of this MR (and by extension future dxil patches) should depend on that (and by extension the 1.9 release).
Which parts of the system are you considering when you make that evaluation? Introducing SSA nodes is certainly simpler if you consider dxil.c in isolation, or even the dxil -> spirv path in isolation. That's (trivially) not true when you consider e.g. d3d_asm.c or spirv.c in isolation. Similarly, SSA nodes are extra work for someone adding e.g. a MSL or GLSL backend. What about transformation passes in ir.c? Are those potentially going to need to handle VKD3DSPR_SSA? We don't run any of the existing ones on dxil sources, but that doesn't make the consideration just go away.
As I said, I agree with Henri that it would have been sensible to first use temporaries, but as a side consideration if we eventually introduce SSA I don't expect that necessarily every consumer of VSIR is expected to handle it. We could internally have a few different dialects, for example one that allows SSA and one that doesn't. And maybe a pass that lowers SSA to temporaries, so that a SSA-less consumer can be chained with a SSA-ful producer. Maybe even a pass that raises temporaries to SSA (i.e., a copy propagation pass), if that turns out to be useful.
Sure; I don't think anyone raised concerns about introducing SSA nodes per se. The issue is mostly about whether the rest of this MR (and by extension future dxil patches) should depend on that (and by extension the 1.9 release).
That makes more sense to me. Personally I wasn't even considering 1.9, and if I was, I would figure that if we're late enough in the release process to be that concerned, we wouldn't want to accept this anyway. I can see why we'd accept isolated changes to the sm6 stuff, though.
[I also did not consider GLSL or MSL, although having done so, I feel like we can translate those to an appropriately-namespaced temp? I.e. it shouldn't be that hard? It's not taking advantage of the SSA part, but it'd at least work.]
I think there is a bit of a tradeoff here, if we do use SSA eventually... namely, the unused complexity it adds to vsir, versus the cost of asking Conor to rewrite his patches just so he can un-rewrite them later. I'm still inclined to say the latter outweighs the former, and if that means we have to wait for 1.9 so be it. But I won't belabor the argument.
I think there is a bit of a tradeoff here, if we do use SSA eventually... namely, the unused complexity it adds to vsir, versus the cost of asking Conor to rewrite his patches just so he can un-rewrite them later. I'm still inclined to say the latter outweighs the former, and if that means we have to wait for 1.9 so be it. But I won't belabor the argument.
Right, at this point the likely scenario is that we'll release 1.9 without dxil support, and then commit this MR more or less as-is after the release. That's unfortunate though, and I don't think it necessarily had to be that way. Perhaps more importantly, I'd also like to make sure we try to avoid similar situations going forward.
I didn't see DXIL support as being particularly relevant to 1.9 because it would be the barest minimum, but I realise it's meaningful as a milestone to have a test pass. Are there other reasons I'm not aware of?
Having just dealt with temps in CFGs, I think SSA -> temp conversion may be reasonable to implement, with a risk of getting caught up in edge cases. It would allow deletion of all the phi complications in the structuriser, which may be more than enough to justify it. It could also break the structuriser and make the project take a lot longer than expected. Also, we could clean up the SPIR-V code by emitting all temp access chains in the entry block with a build-once function, and name them, e.g.
r0.x_ptr
. Then optimise away many loads/stores by tracking and reusing result values within blocks. This is just to make debugging easier.If I test this before the 1.9 release and it works out okay then SSA can be removed from the MR and it will be ready right after 1.9. But the shader runner tests come first.
I didn't see DXIL support as being particularly relevant to 1.9 because it would be the barest minimum, but I realise it's meaningful as a milestone to have a test pass. Are there other reasons I'm not aware of?
Fundamentally, we'd like development to happen upstream as much as possible. There are situations where developing/prototyping something on a branch first makes sense, and this is certainly one of them, but there are costs to maintaining branches. The most obvious of those is perhaps the time spent rebasing to account for upstream changes, but you can also think about e.g. the branch diverging from what's acceptable upstream, people spending time debugging issues that are already fixed on the branch, different people independently writing the same patches, and so on. I.e., we'd simply like to minimise the set of changes that's not upstream as soon as possible. And to give some sense of the kind of upstreaming rate I'd look for, I think for about 80 percent or so of this branch, about 20 patches a week on average should be achievable.
I have a working implementation which uses temps instead of SSA. It runs HZD, but Cyberpunk hangs at the start, which would be very difficult to debug.
It's unoptimised, so compile times are very slow, and it uses more temps than strictly necessary. The latter is partly because it allocates a new one for every phi, because reusing those is more complicated than for other values, and I haven't implemented it. Also, there seems to be a bug in RADV. Some shader double tests fail unless all temps are based at component 0 (doubles are otherwise always aligned at 0 or 2). The generated vsir is valid and I see nothing wrong with the SPIR-V either, plus it triggers failure of later tests which are unchanged by the workaround.
Addition of native 16-bit types poses another problem. Storing them in 32-bit temps may harm performance. We could add a
TEMP16
register type, but we wanted to avoid adding register types. A similar issue exists with bools, but storing them as uint 0 or 1 stands a good chance of being optimised out.Using temps saves 800-900 lines in the structuriser, but performance optimisation and proper allocation of phi temps will add more code.
Temps seem to hit lesser-used and more bug-prone driver code paths. Overall I think this approach is far more trouble than it's worth.
To me, all these sound great reasons to eventually have and use SSA, but not necessarily to have it immediately in the first MRs. I would have suggested starting upstreaming using temporaries, so that changes are smaller and easier to review, and then at some point upgrade to SSA, probably after at least some idea of where the SM6 parser is going (i.e., at least some operations and control flow is supported, some tests are passing).
It wouldn't make anything smaller except for eliminating the SSA patch, and that patch is now only 57 lines. Delaying it only adds extra work later to switch over.
To add to what I wrote earlier about using temps for branched shaders, a very useful feature of phi instructions is the validation Vulkan performs. Writing temps in place of phi incomings is unvalidated, and any issues will result in silent failure of shaders to execute correctly.