On 08/29/2017 05:35 PM, Matteo Bruni wrote:
Right, I think checking that during parsing is a good idea regardless of everything else.
I'd argue that either the expression above is too generic (allowing / handling register_count > 1) or the other register_count expression (the merge one I brought up earlier) is too naive, not expecting the same. As it is it just doesn't look right.
I updated the expression in 'merge' phase by now to do the check, more exactly, added a check for the case under discussion inside the 'if'.
To block the situation when register components exceed register size, I actually should check not just component count but offset also. Perhaps this should also be done not just for output but for input registers only. This can't be done cleanly for input registers though for all the cases, as there might be a relative offset which can still effectively make a multicomponent operation cross register boundary. Not sure if we should add runtime checks for that. Native implementation seems to allow all this and does not refuse to parse preshaders which crossses a register boundary in instruction even without relative addressing. It does allow some things we already deny now, so I think I should improve the checks for components during parsing and block them early when possible, and after that consider preshader instruction output to affect just one register always in this part.
If you ensure during parsing that, for each preshader instruction, 0 < component_count <= components_per_register then you can just always set the initial register_count to 1. Actually you could avoid setting it at all at that point (more on that below).
So maybe I will add an assert for the overlapping registers in this'if' under discussion just in case?
That may also be a good idea. Otherwise, if the "+ out->const_set[i + 1].register_count" part is replaced with a "+ 1", the overlap case would be handled trivially. That would make it possible to avoid the previous register_count initialization entirely, assuming register_count is set to 1 during merging in the NOT merging case.
Yes, sure.
Both options are okay with me. Also, as usual, let me know if I'm still missing something.
I added a check in this patch by now (as all the other code actually allows for multicomponent operation to cross register boundary by now, and thus register_count can potentially exceed 1 here).