[Mesa-dev] [PATCH 092/133] nir: Add a pass to lower local variables to registers

[Connor Abbott]

On Fri, Jan 9, 2015 at 8:27 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>
>
> On Fri, Jan 9, 2015 at 4:38 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>>
>> >>>> +      case nir_intrinsic_copy_var:
>> >>>> +         unreachable("There should be no copies whatsoever at this
>> >>>> point");
>> >>>> +         break;
>> >>>
>> >>>
>> >>> Are you sure about this? My impression is that lower_variables will
>> >>> lower
>> >>> copies involving things that aren't indirectly referenced, but if you
>> >>> have
>> >>> something like:
>> >>>
>> >>> foo[i] = ...
>> >>> bar[*] = foo[*];
>> >>> ... = bar[i];
>> >>>
>> >>> then the copy in the middle won't get lowered, unless there's
>> >>> something
>> >>> else I'm missing that will lower it.
>> >>
>> >>
>> >> Yeah, there may be something missing there.  I have a pass lying around
>> >> somewhere that lowers all copies.  Unfortunately, I've never actually
>> >> seen
>> >> this happen in the wild so It's untested.  I'll try and cook something
>> >> up
>> >> that I think is reliable.
>> >
>> >
>> > Ok, more info.  Right now, GLSL IR is lowering all truely indirect
>> > accesses
>> > to if-ladders right now so we can never hit this.  Once we can handle
>> > indirects in the backends or generate the if-ladders in NIR, we will
>> > need
>> > this.  Until then, let's leave it as-is to reduce the ammount of
>> > untested
>> > code.
>> > --Jason
>>
>> Huh, that's weird... it lowers input/output indirect accesses to
>> if-ladders, but not temporary indirect references, right? So wouldn't
>> something like:
>
>
> It appears to lower locals as well.
>
>>
>>
>> uniform int i;
>> uniform bool f; /* false */
>> in vec4 in_array[20];
>> out vec4 color;
>>
>> void main()
>> {
>>    vec4 foo[20], bar[20], temp[20];
>>    foo = in_array;
>>    foo[i] = 0.0f;
>>    while (f) {
>>       temp = foo;
>>       temp[i] = 1.0f;
>>       foo = bar;
>>       bar = temp;
>>    }
>>    bar[i] = 2.0f;
>>    color = bar[0];
>> }
>>
>> where GLSL IR can't eliminate the copies and doesn't turn the indirect
>> references into if-ladders, and we can't lower them to SSA values,
>> trigger the bug? If GLSL did turn everything into if-ladders, then
>> this entire pass as well as half of lower_variables would be dead
>> code...
>
>
> I think we have a lot of dead code...
>

That's for sure! I thought about this a little more, and while you may
not like it, I think we should still have the code to lower the
remaining wildcard copies, even if it's dead code for now. Right now,
I'd say more than half of the variable lowering code is currently
dead, so I'm not so swayed by the "but it's more dead code!" argument
-- we already have a *lot* of dead code, so let's at least fix a known
bug in it rather than making it even more confusing. I think that the
only way it would make sense to not go all the way is if we stripped
out everything else, but that doesn't make that much sense after I
went through all this work to review it all.