[Mesa-dev] [PATCH 4/4] i965: Properly handle integer types in opt_vector_float().

[Iago Toral]

On Mon, 2016-04-18 at 16:54 -0700, Kenneth Graunke wrote:
> On Monday, April 18, 2016 4:20:50 PM PDT Iago Toral wrote:
> > On Sun, 2016-04-17 at 23:14 -0700, Kenneth Graunke wrote:
> > > Previously, opt_vector_float() always interpreted MOV sources as
> > > floating point, and always created a MOV with a F-type destination.
> > > 
> > > This meant that we could mess up sequences of integer loads, such as:
> > > 
> > >    mov vgrf6.0.x:D, 0D
> > >    mov vgrf6.0.y:D, 1D
> > >    mov vgrf6.0.z:D, 2D
> > >    mov vgrf6.0.w:D, 3D
> > > 
> > > Here, integer 0/1/2/3 become approximately 0.0f, so we generated:
> > > 
> > >    mov vgrf6.0:F, [0F, 0F, 0F, 0F]
> > > 
> > > which is clearly wrong.  We can properly handle this by converting
> > > integer values to float (rather than bitcasting), and emitting a type
> > > converting MOV:
> > > 
> > >    mov vgrf6.0:D, [0F, 1F, 2F, 3F]
> > > 
> > > To do this, see first see if the integer values (converted to float)
> > > are representable.  If so, we use a D-type MOV.  If not, we then try
> > > the floating point values and an F-type MOV.  We make zero not impose
> > > type restrictions.  This is important because 0D would imply a D-type
> > > MOV, but is often used in sequences such as MOV 0D, MOV 0x3f800000D,
> > > where we want to use an F-type MOV.
> > > 
> > > Fixes about 54 dEQP-GLES2 failures with the vec4 VS backend.  This
> > > recently became visible due to changes in opt_vector_float() which
> > > made it optimize more cases, but it was a pre-existing bug.
> > > 
> > > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_vec4.cpp | 24 ++++++++++++++++++++----
> > >  1 file changed, 20 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/
> dri/i965/brw_vec4.cpp
> > > index 12c3c66..2bdcf1f 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
> > > +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> > > @@ -361,9 +361,11 @@ vec4_visitor::opt_vector_float()
> > >     int inst_count = 0;
> > >     vec4_instruction *imm_inst[4];
> > >     unsigned writemask = 0;
> > > +   enum brw_reg_type dest_type = BRW_REGISTER_TYPE_F;
> > >  
> > >     foreach_block_and_inst_safe(block, vec4_instruction, inst, cfg) {
> > >        int vf = -1;
> > > +      enum brw_reg_type need_type;
> > >  
> > >        /* Look for unconditional MOVs from an immediate with a partial
> > >         * writemask.  Skip type-conversion MOVs other than integer 0,
> > > @@ -375,14 +377,26 @@ vec4_visitor::opt_vector_float()
> > >            inst->predicate == BRW_PREDICATE_NONE &&
> > >            inst->dst.writemask != WRITEMASK_XYZW &&
> > >            (inst->src[0].type == inst->dst.type || inst->src[0].d == 0)) {
> > > -         vf = brw_float_to_vf(inst->src[0].f);
> > > +         vf = brw_float_to_vf(inst->src[0].d);
> > > +         need_type = BRW_REGISTER_TYPE_D;
> > > +
> > > +         if (vf == -1) {
> > > +            vf = brw_float_to_vf(inst->src[0].f);
> > > +            need_type = BRW_REGISTER_TYPE_F;
> > > +         }
> > 
> > If we are packing actual float values (not integers), doesn't this mean
> > that we re-interpret them as integers and convert the re-interpreted
> > integer value to float? If the result of that sequence of operations is
> > representable it seems that we would just use a D-MOV from a float that
> > no longer represents the original value, right?
> > 
> > Example:
> > 
> > .f = 5.27 (0x40a8a3d7)
> > .d = 1084793815 (0x40a8a3d7)
> > 
> > so we would do brw_float_to_vf(1084793815.0) instead of
> > brw_float_to_vf(5.27), which does not look right.
> 
> No, I believe this should work.
> 
> Patch 3 makes us stop considering type-converting MOVs.  So, whatever
> MOVs we're looking at will be F -> F or D -> D.
> 
> If we had
> 
>    mov(8) dst<1>D 1084793815D
> 
> it would first try to do:
> 
>    vf = brw_float_to_vf(inst->src[0].d);
> 
> Since brw_float_to_vf takes a float parameter, this is actually:
> 
>    vf = brw_float_to_vf((float) inst->src[0].d);
> 
>    (this might have been unclear, sorry!)
> 
> So our example would try:
> 
>    vf = brw_float_to_vf(1084793815.0f);
> 
> If this were representable (it isn't), then we would generate:
> 
>    mov(8) dst<1>D [1084793815.0f, ...]VF
> 
> Because it's a type converting MOV, the 1084793815.0f source would be
> converted to integer 1084793815 and stored.  Reading dst<8,8,1>F would
> then read 5.27, as expected.

Ah, yes, I was troubled by the conversions and didn't realize that in
the end you produce a type converting MOV again to undo that. Although
you  already explain this in the commit log, maybe adding a small
comment right before we do brw_float_to_vf(inst->src[0].d) clarifying it
might be a good idea.

I suppose you'll send a new version with the fix for the issue reported
by Matt, feel free to CC me when you send it for review.

Iago