- Patch 6/7 ("vkd3d-shader/hlsl: Always load from a synthetic copy in add_load_component().") is the slightly iffy one to me. It makes me nervous that we're generating LOAD instructions during parsing. On the other hand it's not hard to see that it never gets called in a way that can put those loads on the LHS, so I don't hate it.
I don't think that generating loads during parsing is inherently bad. In my opinion what is bad is to use the loads' derefs for generating the stores' lhs derefs, which is what we are getting rid of here.
Agreed on all counts, it's just a matter of being careful that we never *do* get there.
Perhaps in the future we might even consider using the lower_index_loads pass together with the constant folding passes on the rules for every `statement` and other non-terminal symbols.
I don't think I see the idea here?
- Instead of worrying about matrix majority for indices, can we just say that HLSL_IR_INDEX ignores majority and always returns columns (or is it rows?), and rely on lower_index_loads() to split those indexes into multiple loads if necessary?
In HLSL, indexing a matrix always accesses a row. But by necessity our deref paths expect to access rows first for row-major matrices and columns first for column major matrices.
In this implementation, hlsl_init_deref_from_index_chain() expects the indexes of row-major matrices to be `is_column_indexing = false` and the indexes of column-major matrices to be `is_column_indexing = true`.
When parsing array indexings, the hlsl_ir_indexes are always created with `is_column_indexing = false`, and lower_index_loads() takes care of transforming one row load into several column loads for column-major matrices, before creating the final loads.
However, this is not enough for storing to a matrix row, where the index chain must be used immediately to generate the deref. For cases such as these, index instructions are created with `is_column_indexing = true` for hlsl_init_deref_from_index_chain() to work (which is implemented in 7/7).
These are also used to simplify the implementation of lower_index_loads() and add_expr() which otherwise would have to load and store one component at the time, that's if we insert the logic for swapping the last two nodes in the deref path in hlsl_init_deref_from_index_chain() for column-major matrices.
I can spend some time checking how an implementation without the `is_column_indexing` field would work. It would remove the argument in hlsl_new_index but it may be at the expense of less simplicity in these hlsl_init_deref_from_index_chain() callers.
Right, we'd have to split it up on the lhs too. And yes, it means that hlsl_init_deref_from_index_chain() can't exactly exist as it is. It still seems like the right approach to me, but I'm willing to be proven wrong...