On Mon, Apr 18, 2016 at 7:20 AM, Iago Toral <ito...@igalia.com> 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 <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?
I believe you're correct, but that there are no values for which this could cause a problem. Since the case we're trying to handle is loading, e.g., integer <1,2,3,4> as a MOV dst:D, [1,2,3,4]VF it might be safer (if there are in fact problematic values) and clearer to only attempt to reinterpret the bits as as integer if the destination type is an integer type. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev