On Feb 20, 2015 9:27 AM, "Matt Turner" <<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a>> wrote: > > On Fri, Feb 20, 2015 at 9:23 AM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote: > > > > > > On Thu, Feb 19, 2015 at 11:03 PM, Matt Turner <<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a>> wrote: > >> > >> On Thu, Feb 19, 2015 at 11:01 PM, Connor Abbott <<a href="mailto:cwabbott0@gmail.com">cwabbott0@gmail.com</a>> > >> wrote: > >> > I agree with Ken that the regressions are small enough, and it seems > >> > they're mostly stuff we can prevent by being smarter when doing the > >> > sel peephole, so it seems like the cleanup that will probably help > >> > other passes is worth it. > >> > >> So, usually we do that as a preparatory patch. Why aren't we doing that > >> here? > > > > > > Do what in a preparatory patch? Fix up the sel peephole to be able to > > handle "if (foo) bar = baz;"? Sure, I can put that patch together. > > I thought that this would get turned into a conditional select as a > side-effect of going out of SSA anyway (assuming baz was > unconditionally set before the if)? Are these shaders not setting baz > unconditionally? The NIR peephole only triggers if the only instructions in the if or else are moves. In this particular case, it was a uniform load. Before the change, it was lowered to moves in both sides of the if and, since it ended up being a push constant, got peepholed. After the change, it was lowered to moves on only one side of the if (which is theoretically better) and the select didn't trigger. > >> > >> NIR instruction counts is not the metric we care about. > > > > > > No, but cleaning things up means that we can do other optimizations better. > > Also, in each of those cases, the non-ssa NIR code was better it was just > > less cleanable by the backend. We need to work on that, but I don't think > > it's an indicator of a problem. > > --Jason > > Okay, that seems fine. I wouldn't bother updating the SEL peephole. It > sounds like a number of the regressions we have are related to it not > handling something so we might need to do a larger evaluation. Yeah, it just needs to handle one-sided it's better. --Jason