Fix issue with register spilling in light list shaders

[FrancescoC-unity]

On FXC using local arrays often causes the use of "indexable temp" which causes fairly awkward code gen and register spilling as consequence. This is horrible for performance, but most importantly on DX12 on Xbox register spilling is illegal in async, causing hard crashes.

This PR replaces those with LDS and that fixes the issue. On platforms that don't generate bad code like this MIGHT be slightly more expensive (doesn't look like it will be), but it is gonna be better on above mentioned platform... and it doesn't crash :)

For both case I tried to port the code as closely as possible to be using LDS instead of local arrays, not sure if we have more optimal ways, but did not want to introduce potentially more disruptive changes.

Fogbugz link: https://fogbugz.unity3d.com/f/cases/1296144/

[EvgeniiG]

What's the cost of doing this? Z-binning heavily uses the existing code. Is it 5% slower? 50%? You need to profile these changes in the z-binning branch.

[FrancescoC-unity]

What's the cost of doing this? Z-binning heavily uses the existing code. Is it 5% slower? 50%? You need to profile these changes in the z-binning branch.

I tried really briefly and the merging didn't go great, unfortunately don't have time to check on the zbinning branch (I am off from tonight).

On master the AABB part, I just tried and it is actually faster on PS4 too and I had a lot of lights; did not expect that so I guess bad code gen was made there too

[sebastienlagarde]

What's the cost of doing this? Z-binning heavily uses the existing code. Is it 5% slower? 50%? You need to profile these changes in the z-binning branch.

Note that the current code crash on Xbox (d3d12) and is shipped with HDRP 10.x and 11.x. So we must fix it in all cases, it can't stay as it was and must be backport to current shipped verison. Mainline is the priority. We missed it as d3d12 xbox test have only been added recently on Yamato.

[EvgeniiG]

In the current version of the HDRP, the perf impact will be minimal for sure, so it is safe to merge. But in the new version, the perf impact may be unacceptable, so I don't know if it's a great long-term solution. Shared memory is generally far slower than registers (100 cycles to access). I must also note that this is an Xbox-specific bug, yet this change affects ALL platforms.

[FrancescoC-unity]

In the current version of the HDRP, the perf impact will be minimal for sure, so it is safe to merge. But in the new version, the perf impact may be unacceptable, so I don't know if it's a great long-term solution. Shared memory is generally far slower than registers (100 cycles to access).

Yes, but it spills on FXC, so it will be slower than LDS access in that case and it will explode VGPR usage. This is not xbox specific is FXC specific. It is just that xbox is more strict. Indeed worth checking what happens on other platforms and if bad then we make specific for fxc platforms.

The change can be made per platform, but it would pollute a fair bit the code given the type of change.

[EvgeniiG]

The spill is to LDS I assume. So the compiler is doing something like you did, just less aggressive.

[EvgeniiG]

I tried really briefly and the merging didn't go great, unfortunately don't have time to check on the zbinning branch (I am off from tonight).

On master the AABB part, I just tried and it is actually faster on PS4 too and I had a lot of lights; did not expect that so I guess bad code gen was made there too

OK, I will profile it for you.

[FrancescoC-unity]

The spill is to LDS I assume. So the compiler is doing something like you did, just less aggressive.

Sending message to slack. But no, it absolutely does not, it is going to be much slower (more detail privately).