[Mesa-dev] r600g/sb: Don't fold integer value into float CND

Submitted by Glenn Kennard on Feb. 12, 2015, 1:23 p.m.

Details

Message ID 1423747418-24420-1-git-send-email-glenn.kennard@gmail.com
State New, archived
Headers show

Not browsing as part of any series.

Commit Message

Glenn Kennard Feb. 12, 2015, 1:23 p.m.
Don't try to do float comparisons on signed integer values,
some of them look like NaNs.

Fixes fs-temp-array-mat3-index-col-row-rd.shader_test regression
caused by 0d4272cd8e7c45157140dc8e283707714a8238d5.

Signed-off-by: Glenn Kennard <glenn.kennard@gmail.com>
---
 src/gallium/drivers/r600/sb/sb_peephole.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/r600/sb/sb_peephole.cpp b/src/gallium/drivers/r600/sb/sb_peephole.cpp
index d4b9755..4161d59 100644
--- a/src/gallium/drivers/r600/sb/sb_peephole.cpp
+++ b/src/gallium/drivers/r600/sb/sb_peephole.cpp
@@ -250,7 +250,7 @@  void peephole::optimize_CNDcc_op(alu_node* a) {
 		return;
 
 	// TODO we can handle some cases for uint comparison
-	if (dcmp_type == AF_UINT_CMP)
+	if (dcmp_type == AF_UINT_CMP || dcmp_type == AF_INT_CMP)
 		return;
 
 	if (dcc == AF_CC_NE) {

Comments

Op 12-02-15 om 14:23 schreef Glenn Kennard:
> Don't try to do float comparisons on signed integer values,
> some of them look like NaNs.
>
> Fixes fs-temp-array-mat3-index-col-row-rd.shader_test regression
> caused by 0d4272cd8e7c45157140dc8e283707714a8238d5.
>
> Signed-off-by: Glenn Kennard <glenn.kennard@gmail.com>
>
Cc: "10.5" <mesa-stable@lists.freedesktop.org>
Reported-andTested-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
On 12 February 2015 at 23:49, Maarten Lankhorst
<maarten.lankhorst@canonical.com> wrote:
> Op 12-02-15 om 14:23 schreef Glenn Kennard:
>> Don't try to do float comparisons on signed integer values,
>> some of them look like NaNs.
>>
>> Fixes fs-temp-array-mat3-index-col-row-rd.shader_test regression
>> caused by 0d4272cd8e7c45157140dc8e283707714a8238d5.
>>
>> Signed-off-by: Glenn Kennard <glenn.kennard@gmail.com>
>>
> Cc: "10.5" <mesa-stable@lists.freedesktop.org>
> Reported-andTested-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>

This is certinaly right and leads to a problem later on though,

tests/spec/glsl-1.10/execution/variable-indexing/fs-uniform-array-mat3-col-row-rd.shader_test
regresses as pointed out,

however the TGSI generated for this as an uninitialised variable in it

FRAG
PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
DCL OUT[0], COLOR
DCL CONST[0..8]
DCL TEMP[0..2], LOCAL
DCL ADDR[0]
IMM[0] INT32 {0, 1, 2, 0}
IMM[1] FLT32 {    0.0000,     1.0000,     0.0000,     0.0000}
  0: UARL ADDR[0].x, CONST[0].xxxx
  1: MOV TEMP[0].xyz, CONST[ADDR[0].x+6].xyzx
  2: USEQ TEMP[1].xyz, CONST[1].xxxx, IMM[0].xyzz
  3: UCMP TEMP[2].x, TEMP[1].xxxx, TEMP[0].xxxx, TEMP[2].xxxx
  4: UCMP TEMP[2].x, TEMP[1].yyyy, TEMP[0].yyyy, TEMP[2].xxxx
  5: UCMP TEMP[2].x, TEMP[1].zzzz, TEMP[0].zzzz, TEMP[2].xxxx
  6: FSEQ TEMP[0].x, TEMP[2].xxxx, CONST[2].xxxx
  7: UIF TEMP[0].xxxx :0
  8:   MOV TEMP[0], IMM[1].xyxy
  9: ELSE :0
 10:   MOV TEMP[0], IMM[1].yxxy
 11: ENDIF
 12: MOV OUT[0], TEMP[0]
 13: END

note TEMP[2] is used a src before being used a dst

the GLSL for this
GLSL IR for linked fragment program 3:
(
(declare (shader_out ) vec4 gl_FragColor)
(declare (temporary ) vec4 gl_FragColor)
(declare (uniform ) int col)
(declare (uniform ) int row)
(declare (uniform ) float expect)
(declare (uniform ) (array mat3 2) m)
(function main
  (signature void
    (parameters
    )
    (
      (declare (temporary ) vec4 conditional_tmp)
      (declare (temporary ) vec3 vec_value_tmp)
      (assign  (xyz) (var_ref vec_value_tmp)  (array_ref (array_ref
(var_ref m) (constant int (1)) ) (var_ref col) ) )
      (declare (temporary ) float vec_index_tmp_v)
      (declare (temporary ) bvec3 dereference_condition)
      (assign  (xyz) (var_ref dereference_condition)  (expression
bvec3 == (swiz xxx (var_ref row) )(constant ivec3 (0 1 2)) ) )
      (assign (swiz x (var_ref dereference_condition) ) (x) (var_ref
vec_index_tmp_v)  (swiz x (var_ref vec_value_tmp) ))
      (assign (swiz y (var_ref dereference_condition) ) (x) (var_ref
vec_index_tmp_v)  (swiz y (var_ref vec_value_tmp) ))
      (assign (swiz z (var_ref dereference_condition) ) (x) (var_ref
vec_index_tmp_v)  (swiz z (var_ref vec_value_tmp) ))
      (if (expression bool == (var_ref vec_index_tmp_v) (var_ref expect) ) (
        (assign  (xyzw) (var_ref conditional_tmp)  (constant vec4
(0.000000 1.000000 0.000000 1.000000)) )
      )
      (
        (assign  (xyzw) (var_ref conditional_tmp)  (constant vec4
(1.000000 0.000000 0.000000 1.000000)) )
      ))

      (assign  (xyzw) (var_ref gl_FragColor)  (var_ref conditional_tmp) )
      (assign  (xyzw) (var_ref gl_FragColor@3)  (var_ref gl_FragColor) )
    ))

)

I'm having trouble deciphering :-) and it comes from
lower_vec_index_to_cond_assign.cpp.

This is probably a glsl->tgsi convertor bug, any GLSL expert say if
the GLSL is okay?

Dave.
What does the source look like? Both the TGSI and GLSL looks fine to
me. This is essentially doing:

vec3 vec_value_tmp = ...;
bvec3 dereference_condition = equals(row.xxx, vec3(0, 1, 2));
//per-component equals
float tmp; //called vec_index_tmp_v in the GLSL IR
// this is where the use-before-def comes in -- note this is a
conditional move in GLSL IR and UCMP in TGSI
tmp = dereference_condition.x ? vec_value_tmp.x : tmp;
tmp = dereference_condition.y ? vec_value_tmp.y : tmp;
tmp = dereference_condition.z ? vec_value_tmp.z : tmp;

which the compiler lowered from:

tmp = vec_value_tmp[row];

Note that while we do use tmp before we store it, we're guaranteed to
have a valid value for tmp by the time we reach the end since in the
original source code row was used as an index (and we can give
undefined results if row is out-of-range). So it seems like it's a
driver bug.


On Thu, Feb 12, 2015 at 11:30 PM, Dave Airlie <airlied@gmail.com> wrote:
> On 12 February 2015 at 23:49, Maarten Lankhorst
> <maarten.lankhorst@canonical.com> wrote:
>> Op 12-02-15 om 14:23 schreef Glenn Kennard:
>>> Don't try to do float comparisons on signed integer values,
>>> some of them look like NaNs.
>>>
>>> Fixes fs-temp-array-mat3-index-col-row-rd.shader_test regression
>>> caused by 0d4272cd8e7c45157140dc8e283707714a8238d5.
>>>
>>> Signed-off-by: Glenn Kennard <glenn.kennard@gmail.com>
>>>
>> Cc: "10.5" <mesa-stable@lists.freedesktop.org>
>> Reported-andTested-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
>
> This is certinaly right and leads to a problem later on though,
>
> tests/spec/glsl-1.10/execution/variable-indexing/fs-uniform-array-mat3-col-row-rd.shader_test
> regresses as pointed out,
>
> however the TGSI generated for this as an uninitialised variable in it
>
> FRAG
> PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
> DCL OUT[0], COLOR
> DCL CONST[0..8]
> DCL TEMP[0..2], LOCAL
> DCL ADDR[0]
> IMM[0] INT32 {0, 1, 2, 0}
> IMM[1] FLT32 {    0.0000,     1.0000,     0.0000,     0.0000}
>   0: UARL ADDR[0].x, CONST[0].xxxx
>   1: MOV TEMP[0].xyz, CONST[ADDR[0].x+6].xyzx
>   2: USEQ TEMP[1].xyz, CONST[1].xxxx, IMM[0].xyzz
>   3: UCMP TEMP[2].x, TEMP[1].xxxx, TEMP[0].xxxx, TEMP[2].xxxx
>   4: UCMP TEMP[2].x, TEMP[1].yyyy, TEMP[0].yyyy, TEMP[2].xxxx
>   5: UCMP TEMP[2].x, TEMP[1].zzzz, TEMP[0].zzzz, TEMP[2].xxxx
>   6: FSEQ TEMP[0].x, TEMP[2].xxxx, CONST[2].xxxx
>   7: UIF TEMP[0].xxxx :0
>   8:   MOV TEMP[0], IMM[1].xyxy
>   9: ELSE :0
>  10:   MOV TEMP[0], IMM[1].yxxy
>  11: ENDIF
>  12: MOV OUT[0], TEMP[0]
>  13: END
>
> note TEMP[2] is used a src before being used a dst
>
> the GLSL for this
> GLSL IR for linked fragment program 3:
> (
> (declare (shader_out ) vec4 gl_FragColor)
> (declare (temporary ) vec4 gl_FragColor)
> (declare (uniform ) int col)
> (declare (uniform ) int row)
> (declare (uniform ) float expect)
> (declare (uniform ) (array mat3 2) m)
> (function main
>   (signature void
>     (parameters
>     )
>     (
>       (declare (temporary ) vec4 conditional_tmp)
>       (declare (temporary ) vec3 vec_value_tmp)
>       (assign  (xyz) (var_ref vec_value_tmp)  (array_ref (array_ref
> (var_ref m) (constant int (1)) ) (var_ref col) ) )
>       (declare (temporary ) float vec_index_tmp_v)
>       (declare (temporary ) bvec3 dereference_condition)
>       (assign  (xyz) (var_ref dereference_condition)  (expression
> bvec3 == (swiz xxx (var_ref row) )(constant ivec3 (0 1 2)) ) )
>       (assign (swiz x (var_ref dereference_condition) ) (x) (var_ref
> vec_index_tmp_v)  (swiz x (var_ref vec_value_tmp) ))
>       (assign (swiz y (var_ref dereference_condition) ) (x) (var_ref
> vec_index_tmp_v)  (swiz y (var_ref vec_value_tmp) ))
>       (assign (swiz z (var_ref dereference_condition) ) (x) (var_ref
> vec_index_tmp_v)  (swiz z (var_ref vec_value_tmp) ))
>       (if (expression bool == (var_ref vec_index_tmp_v) (var_ref expect) ) (
>         (assign  (xyzw) (var_ref conditional_tmp)  (constant vec4
> (0.000000 1.000000 0.000000 1.000000)) )
>       )
>       (
>         (assign  (xyzw) (var_ref conditional_tmp)  (constant vec4
> (1.000000 0.000000 0.000000 1.000000)) )
>       ))
>
>       (assign  (xyzw) (var_ref gl_FragColor)  (var_ref conditional_tmp) )
>       (assign  (xyzw) (var_ref gl_FragColor@3)  (var_ref gl_FragColor) )
>     ))
>
> )
>
> I'm having trouble deciphering :-) and it comes from
> lower_vec_index_to_cond_assign.cpp.
>
> This is probably a glsl->tgsi convertor bug, any GLSL expert say if
> the GLSL is okay?
>
> Dave.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev