From: Zebediah Figura Subject: Re: HLSL offsetting Message-Id: Date: Mon, 27 Jun 2022 17:05:18 -0500 In-Reply-To: References: <6a65d783-32b7-1869-622f-2898f70af302@codeweavers.com> <62467a7d-3a59-59b8-ef11-ec7b681f7e5d@codeweavers.com> <32508591-b482-cfb2-ef9d-afa0af351552@codeweavers.com> <145424ee-00e6-c6fa-ba80-83a8faadaa1a@codeweavers.com> <0b9be172-5c4a-b846-30b7-7be58d2ec767@codeweavers.com> <54839a65-9285-647f-836d-4eb074c7995f@codeweavers.com> <7bd6d5b0-0114-56d1-ea30-94ebdd830f8a@codeweavers.com> <1baa6160-f472-e80e-1033-b41c00a4fe73@codeweavers.com> <9a60c866-77fb-73cd-0e5d-26fbcff862a6@codeweavers.com> <58124ceb-c80e-14f8-094d-65edf961e6ae@codeweavers.com> <07dca95c-e258-84bc-be37-dd3236a56ea0@codeweavers.com> 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.