HLSL offsetting

[Zebediah Figura]

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.