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 <kenn...@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. > + /* Zero can be loaded as any type; don't impose a restriction. */ > + if (inst->src[0].d == 0) > + need_type = dest_type; > } > > /* If this wasn't a MOV, or the value was non-representable, or > - * the destination register doesn't match, then this breaks our > - * sequence. Combine anything we've accumulated so far. > + * the destination register doesn't match, or we have to switch > + * destination types, then this breaks our sequence. Combine > + * anything we've accumulated so far. > */ > if (vf == -1 || > + dest_type != need_type || > last_reg != inst->dst.nr || > last_reg_offset != inst->dst.reg_offset || > last_reg_file != inst->dst.file) { > @@ -391,7 +405,7 @@ vec4_visitor::opt_vector_float() > unsigned vf; > memcpy(&vf, imm, sizeof(vf)); > vec4_instruction *mov = MOV(imm_inst[0]->dst, brw_imm_vf(vf)); > - mov->dst.type = BRW_REGISTER_TYPE_F; > + mov->dst.type = dest_type; > mov->dst.writemask = writemask; > inst->insert_before(block, mov); > > @@ -405,6 +419,7 @@ vec4_visitor::opt_vector_float() > inst_count = 0; > last_reg = -1; > writemask = 0; > + dest_type = BRW_REGISTER_TYPE_F; > > for (int i = 0; i < 4; i++) { > imm[i] = 0; > @@ -428,6 +443,7 @@ vec4_visitor::opt_vector_float() > last_reg = inst->dst.nr; > last_reg_offset = inst->dst.reg_offset; > last_reg_file = inst->dst.file; > + dest_type = need_type; > } > } > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev