Hello,
JIC, I attach the most recent version of the patch. I still have to split it, but I addressed all the other suggestions.
Best regards, Francisco.
On 27-06-22 18:05, Zebediah Figura wrote:
On 6/27/22 11:54, Francisco Casas wrote:
Hello,
On 25-06-22 13:29, Zebediah Figura wrote:
On 6/24/22 23:46, Francisco Casas wrote:
Hello,
I attach the third version of the previous patch, applying the suggestions made by Zeb, and some other changes that arose from them.
The patch is based on the master branch, but I guess I will have to update it if the recent patches from Giovanni are accepted, before sending it for review.
Is it not possible to split up this patch, as I had asked earlier?
I think I can separate some small parts, like hlsl_free_deref() as you suggested, but we would still have a large patch that's hard to split.
Sure. Every little bit helps, though.
I think if we introduce hlsl_new_component_load() and hlsl_new_load_from_deref() [or whatever they end up being called] in a separate patch (or patches) that'll do a lot to help review.
There's also a couple things I notice from skimming:
- I think add_load() should also be split into deref / component
versions. Most of its users only need to count components.
add_record_load() and add_array_load() will require indexes, but yes, add_matrix_scalar_load(), intrinsic_mul(), add_expr(), and add_cast(), in particular the parts where they deal with matrices, could be simplified if we use component offsets.
I will split the function then.
- Actually, should we be passing a deref instead of just a var to
hlsl_new_component_load()? See e.g. add_cast(). That would make it a simple wrapper around hlsl_new_load_from_deref() [and the names would also need to be changed...] I'm not sure why I thought it made sense like this, frankly.
Allowing to pass a deref instead of just a var to hlsl_new_component_load() seems like a good idea. It may save us from creating synthetic variables e.g. in initialize_var_components().
I don't think it can be just made a simple wrapper around hlsl_new_load_from_deref() though, since a single component within a struct may require to be accessed with path of larger length than 2. It requires a little more complex implementation, but it can be done.
Ah, right, of course.
- Do we need to always split up matrices into scalars? Should we make
non-contiguous loads a problem for parse time? As long as we're trying to keep backend-specific details out of the HLSL IR, that seems potentially reasonable; it'd just require emitting multiple copy instructions for some cases. That would allow us to simplify split_copy() and add_matrix_scalar_load(), and I believe would thus also get rid of the need to pass multiple offsets to hlsl_new_load_from_deref().
If we go back to make non-contiguous loads a problem for parse time, then I think it would make sense for row_major matrices to retrieve a row, and column_matrices to retrieve a column when indexing with a path.
I will see if I can do it while keeping the component offsets consistent with the order in the initializers.
Right. To be clear I'm not *sure* if this is the best approach, but it seems possible.