[Mesa-dev] [PATCH 2/3] i965: Add runtime checks for line antialiasing in Gen < 6.

[Iago Toral]

On Thu, 2014-05-29 at 08:31 -0700, Kenneth Graunke wrote:
(...)
> >>> +      emit(IF(BRW_PREDICATE_NORMAL));
> >>> +      {
> >>> +         /* Shift message header one register since we are not sending
> >>> +          * AA data stored in base_mrf+2
> >>> +          */
> >>> +         do_emit_fb_write(target, base_mrf + 1, mlen - 1, eot, header_present);
> >>> +      }
> >>> +      emit(BRW_OPCODE_ELSE);
> >>> +      {
> >>> +         do_emit_fb_write(target, base_mrf, mlen, eot, header_present);
> >>> +      }
> >>> +      emit(BRW_OPCODE_ENDIF);
> >>
> >> I suppose this probably works, but I've never seen a program end with an
> >> ENDIF.  I'm not really comfortable with doing that, since the ENDIF
> >> should never be executed.  With JMPI or ADD brw_ip_reg(), we'd just jump
> >> over one instruction and then execute one FB write or the other, which
> >> seems really straightforward.
> >>
> >> But, looking at the bug, I see you tried both of those suggestions, and
> >> it didn't work for some reason.  Huh.  It really should...maybe I can
> >> look into why...
> > 
> > Right, I don't understand why that code did not work for me either. More
> > over, the code did not work as I expected in other ways, for example I
> > noticed that reverting the condition and the order of the fb_writes
> > (which should be completely the same thing) changed the behavior too...
> > I remember that it produced GPU hangs but I tried so many versions that
> > I can't tell... it felt as if the two fb_writes were being executed.  I
> > tried all kinds of things to make it work as intended, even including a
> > manual JMPI at the end of the IF branch to force the jump over the
> > second fb_write... but nothing.
> > 
> > I commented about this and included the C code and the generated
> > assembly for the JMPI version hoping that someone could tell if
> > something was off here:
> > 
> > http://lists.freedesktop.org/archives/mesa-dev/2014-May/059985.html
> > 
> > Maybe you can have a quick look at that see if there is something
> > obvious that I am missing...
> 
> Yeah, I'll try and take a look...

I found what was going on with this. I have to manually set the
execution_size of the JMPI instruction to BRW_EXECUTE_1, that seems to
fix the problem.

> > Also, when I was trying the JMPI version I was developing the patches in
> > the generator rather than the visitor. I suppose that should not make
> > any difference though.
> > 
> > One last thing, JMPI was recently made private to the sf module, so if
> > we want to do go with it we need to revert f3cb2e6ed705.
> 
> It wasn't, actually - brw_JMPI is still available in brw_eu_emit.c.
> It's only brw_land_fwd_jump that moved to brw_sf_state.c, but all that
> does is set the jump distance on the JMPI instruction.  It seems like
> you know the jump distance when emitting the JMPI (jump over 1 FB write
> and maybe some MOVs) so you could just set it straightaway, and skip
> that function.  Or you could revert it - that'd be fine too.

Oh, right... I did not notice that.

Still, brw_land_fwd_jump() seems very convenient and safer to use than
relying on manual calculations for the jump distance (I think that would
be specially error prone if someone has to change the code at a later
point, since they have to know that they need to alter the jump distance
if they add/remove instructions that execute in the conditional) so I
think reverting is the best option.

Iago