<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Jan 14, 2015 at 1:10 PM, Connor Abbott <<a href="mailto:cwabbott0@gmail.com" target="_blank">cwabbott0@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, Jan 9, 2015 at 8:27 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote: > > > On Fri, Jan 9, 2015 at 4:38 PM, Connor Abbott <<a href="mailto:cwabbott0@gmail.com">cwabbott0@gmail.com</a>> 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... > </div></div>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. </blockquote></div> </div><div class="gmail_extra">Agreed. I'll put a pass together. It shouldn't take me long. </div></div>