On Mon, Oct 26, 2015 at 6:56 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Mon, Oct 26, 2015 at 12:53 PM, Rob Clark <robdcl...@gmail.com> wrote:
>> On Mon, Oct 26, 2015 at 2:44 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
>>> On Mon, Oct 26, 2015 at 8:27 AM, Rob Clark <robdcl...@gmail.com> wrote:
>>>>
>>>> +static void *
>>>> +clone_intrinsic(clone_state *state, const void *ptr)
>>>> +{
>>>> + const nir_intrinsic_instr *itr = ptr;
>>>> + unsigned num_srcs = nir_intrinsic_infos[itr->intrinsic].num_srcs;
>>>> +
>>>> + nir_intrinsic_instr *nitr =
>>>> + ralloc_size(state->ns,
>>>> + sizeof(nir_intrinsic_instr) + num_srcs *
>>>> sizeof(nir_src));
>>>> + store_ptr(state, nitr, itr);
>>>> +
>>>> + __clone_instr(state, &nitr->instr, &itr->instr);
>>>> +
>>>> + nitr->intrinsic = itr->intrinsic;
>>>> +
>>>> + if (nir_intrinsic_infos[itr->intrinsic].has_dest)
>>>> + __clone_dst(state, &nitr->dest, &itr->dest);
>>>> +
>>>> + nitr->num_components = itr->num_components;
>>>> + memcpy(nitr->const_index, itr->const_index, sizeof(nitr->const_index));
>>>> +
>>>> + for (unsigned i = 0; i < ARRAY_SIZE(nitr->variables); i++) {
>>>> + nitr->variables[i] = clone_ptr(state, itr->variables[i],
>>>> clone_deref_var);
>>>
>>> Derefs don't need to go in the remap table. They need to use the
>>> variable from it and need to remap any indirect sources, but the deref
>>> doesn't need to go in the table.
>>
>> that goes for all the various places deref's can appear? (Ie. call,
>> tex instr, etc)? They are all private to the instruction? I got a
>> bit lost/impatient in following glsl_to_nir around so I went for the
>> safe thing..
>
> Yes, that's correct. A deref is always only local to the instruction.
> This is why I suggested passing the instruction into clone_deref
> above. If you don't need to use clone_ptr on it, then it doesn't
> require passing extra stuff through.
oh, and I see, and nir_validate actually checks some of this stuff..
(possibly wouldn't hurt for it to validate parent memctx a bit more
thorough.. it took a bit of digging to figure out what should be
parent memctx of what and whether I could completely rely on that..
but other topic..)
I guess things that nir_validate asserts, I'd be more comfortable
relying on, vs letting things get broken until someone bothers to run
a debug build w/ NIR_TEST_CLONE=1 ;-)
>> (That said, in the grand scheme of things I don't think there are so
>> many deref objects, so probably doesn't make much difference)
>
> Sure, but it simplifies things if you just create it on the spot and
> pass the instruction through.
maybe.. otoh less things that are handled differently and require
thought to understand why they are handled in a particular way in
nir_clone, vs other fields/ptrs/etc, makes the code easier to
understand and keep up to date.. or at least that is part of my
reasoning about some things..
>>>> + }
>>>> +
>>>> + for (unsigned i = 0; i < num_srcs; i++) {
>>>> + __clone_src(state, &nitr->src[i], &itr->src[i]);
>>>> + }
>>>> +
>>>> + return nitr;
>>>> +}
>>>> +
[snip]
>>>> +
>>>> +static void
>>>> +__clone_cf_node_p2(clone_state *state, nir_cf_node *ncf, const
>>>> nir_cf_node *cf)
>>>> +{
>>>> + switch (cf->type) {
>>>> + case nir_cf_node_block:
>>>> + __clone_block_p2(state, nir_cf_node_as_block(ncf),
>>>> nir_cf_node_as_block(cf));
>>>> + break;
>>>> + case nir_cf_node_if:
>>>> + __clone_if_p2(state, nir_cf_node_as_if(ncf), nir_cf_node_as_if(cf));
>>>> + break;
>>>> + case nir_cf_node_loop:
>>>> + __clone_loop_p2(state, nir_cf_node_as_loop(ncf),
>>>> nir_cf_node_as_loop(cf));
>>>> + break;
>>>> + case nir_cf_node_function:
>>>> + __clone_function_impl_p2(state, nir_cf_node_as_function(ncf),
>>>> + nir_cf_node_as_function(cf));
>>>
>>> This whole song-and-dance isn't needed. We already have
>>> nir_foreach_block which will do all of this for you. To handle the
>>> ifs, we have nir_block_get_following_if(). See also
>>> nir_live_variables for details.
>>
>> but it won't co-iterate two lists at the same time for me ;-)
>
> Sure, but I'm pretty sure you never use it given my comment on
> clone_block_p2 :-)
well, partially.. I still need some fixup in nir_if too, but given
that I simplified things to just keep a list of nir_src's to fixup, I
also don't need the origin nir_if there currently. (The earlier
version of the patch had more complete ir __clone_foo_v2()'s to
traverse the entire graph to avoid keeping a list of nir_src's to
fixup.)
At any rate, in either case I need to do my own custom traversal to
get at the nir_if's, regardless of whether I need to co-traverse the
original-foo's. Leaving the co-traversal in place at least simplifies
things later if any other place needs to do a 2nd-pass ptr lookup, if
the need arises in the future.
Maybe I should just throw in some some assert(nfoo == clone_ptr(foo))
so we have some use for the co-traversal until then ;-)
BR,
-R
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
