[Mesa-dev] [PATCH 2/2] prog_optimize: Add reads without writes optimization pass

[Tom Stellard]

On Wed, Mar 30, 2011 at 10:09:58AM -0700, Ian Romanick wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 03/29/2011 08:27 PM, Tom Stellard wrote:
> > 
> > The reason there are reads of undefined values is because of all the
> > conditional assignments generated by the IF to conditional assignment
> > lowering pass.  ir_to_mesa transforms conditional assignments to
> > CMP DST, COND SRC0 DST, with the assumption that if the condition fails
> > assigning DST to itself will be a noop.  This is normally a safe assumption
> > to make since all bug-free programs should initialize a value before using
> > it in a conditional assignment, if the value is going to be used later
> > in the program.
> > 
> > However, the conditional assignments that are generated by the IF
> > lowering pass don't follow this pattern and the DST register is usually
> > uninitialized when the instruction is executed, leading to a read from
> > an undefined value.
> 
> Ah, that makes sense.  It would probably be good to replace the examples
> in the current header comment with one of these cases.
> 
> > It seems like the real problem is that there is no good way to translate
> > a GLSL IR conditional assignment to a MESA IR instruction.  From what
> > I can tell, the semantics of conditional assignment are:
> > if (cond)
> > 	assign value
> > else
> > 	do nothing;
> 
> Correct.
> 
> > and the closest equivalent Mesa IR instruction (CMP) is:
> > if (cond)
> > 	assign value0
> > else
> > 	assign value1
> > 
> > Since this is only an issue on architectures that don't support flow
> > control, I should modify my patch so the 'reads without writes' pass
> > only runs on architectures that don't support flow control.  I'm also
> > interested in hearing alternate solutions if anyone has other ideas,
> > because I would really like to get this fixed.
> 
> It seems like there are two possible cases, and this doesn't really fix
> either of them.
> 
> Case 1:
> 
>     if (cond)
>         x = y;
> 
> This gets lowered to a conditional move to x.  If there is no 'else',
> why do a conditional move at all?  In this case, replace the CMP (which
> may be lowered to as many as 3 instructions in some vertex shaders) with
> a MOV.
> 
> If x is used when cond is false before another assignment to x, the
> result is undefined.  Reading y qualifies as much as reading 0s.  Right? :)
> 
> Case 2:
> 
>     if (cond)
>         x = y;
>     else
>         x = z;
> 
> In this case we end up with two CMP instructions.  The first of the CMP
> instructions will be potentially read from an uninitialized source.
> 
>     CMP x, cond, y, x;
>     CMP x, cond, x, z;
> 
> In this case, the two CMP instructions should be fused into a single CMP.
> 
>     CMP x, cond, y, z;
> 
> Now that I think about it, the way the copy propagation works,
> converting the first CMP to a MOV will make this happen automatically.
> 
>     CMP x, cond, y, x;
>     CMP x, cond, x, z;
> 
> becomes
> 
>     MOV x, y;
>     CMP x, cond, x, z;
> 
> becomes
> 
>     MOV x, y;
>     CMP x, cond, y, z;
> 
> becomes
> 
>     CMP x, cond, y, z;

Ok, this makes sense.  It would be pretty easy to modify my original patch
to do this, but maybe it's better change the way ir_to_mesa.cpp converts
conditional assignments to Mesa IR.  Do you have any suggestions for how
to determine if the left hand side of an ir_assignment has already been
written to?

-Tom