Re: [Mesa-dev] [PATCH] st/glsl_to_tgsi: Translate float ir_unop_neg into float MOV with modifier.

2017-03-06 Thread Roland Scheidegger 
Am 06.03.2017 um 22:58 schrieb Francisco Jerez:
> Roland Scheidegger  writes:
> 
>> Am 04.03.2017 um 20:45 schrieb Ilia Mirkin:
>>> Also, how is this happening in the first place? For example, we have:
>>>
>>>case ir_unop_bitcast_f2i:
>>>case ir_unop_bitcast_f2u:
>>>   /* Make sure we don't propagate the negate modifier to integer 
>>> opcodes. */
>>>   if (op[0].negate || op[0].abs)
>>>  emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
>>>   else
>>>  result_src = op[0];
>>>
>>> Oh, but it's going directly into a ir_triop_csel, which is missing
>>> this logic. It should be added there instead, IMHO. OTOH, the same
>>> issue will hit in emit_block_mov() if you do. Would love to hear some
>>> other opinions... Marek, Brian, Roland?
>>
>> float modifiers used with UCMP are actually just fine in theory - the
>> UCMP sources are considered floats for the purposes of modifiers (this
>> is similar to mov which also has untyped sources, except that 1 of the
>> arguments of ucmp indeed is a uint, which cannot have modifiers).
>> (tgsi_opcode_infer_type() says it's untyped, but
>> tgsi_opcode_infer_src_type() says it's float though obviously this
>> doesn't apply to the condition argument.)
>>
> 
> Yikes...  That sounds terribly inconsistent...  Would it make more sense
> to fix UCMP to behave like other integer instructions? (i.e. as the TGSI
> docs claim it works)
Slightly inconsistent or not, I very much like it how it is, and would
rather improve the docs and fix tgsi exec. (Because a) it is probably
more useful that way, since int modifiers are rare, limited to neg and
I'm not sure that is used at all and b) I don't see a good reason to
deviate from dx10 here.)
Yes the docs say "Integer conditional mov" but that's really just
because the condition is an integer (it belongs to the native integer
operations).
(For TGSI_OPCODE_MOV, there's actually some special mention that the
operands count as floats.)

Roland


> 
>> Therefore, not handling float modifiers with ucmp src args is a bug of
>> the tgsi executor (I'm quite sure llvmpipe will handle it right). Looks
>> like all source arguments must be the same type there... Seems annoying
>> to fix actually...
>> Albeit the gallium docs don't really mention this (this is the same
>> behavior as dx10 movc). I don't know though if other drivers will handle
>> it correctly, but if they do it might be a better idea to just fix
>> tgsi_exec somehow.
>>
>> (I'm not sure if the opposite could happen, that is int modifiers
>> mistakenly going into a ucmp, or if this could be a problem with other
>> instructions.)
>>
>> Roland
>>
>>
>>>   -ilia
>>>
>>>
>>> On Sat, Mar 4, 2017 at 2:24 PM, Ilia Mirkin  wrote:
 Hmmm... I wonder if this should only be done for the native_integers
 case. I'm concerned that this will cause perf regressions on weaker hw
 like nv30 and r300, as the neg will no longer be inserted as a
 modifier into the next instruction. Any opinion on this?

 On Sat, Mar 4, 2017 at 2:16 PM, Francisco Jerez  
 wrote:
> Otherwise result_src may be provided to an integer instruction whose
> negate modifier has different semantics.  Example is UCMP as in the
> bug linked below, where an unrelated change in the GLSL built-in
> lowering code for atan2 (e9ffd12827ac11a2d2002a42fa8eb1df847153ba)
> caused the generation of floating-point ir_unop_neg instructions
> followed by ir_triop_csel, which is lowered into UCMP on back-ends
> with native integer support.
>
> Fixes a regression of piglit glsl-fs-tan-1 on softpipe introduced by
> the above-mentioned glsl front-end commit.  Even though the commit
> that triggered the regression doesn't seem to have made it to any
> stable branches yet, this seems worth back-porting since I don't see
> any reason why the bug couldn't have been reproduced before that
> point.
>
> Bugzilla: 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_show-5Fbug.cgi-3Fid-3D99817=DwIGaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04=VpboBTuXbZaDL1-iEDED9JFLZTZFsPVqjTSbTuGSuT8=
>  
> Tested-by: Vinson Lee 
> Cc: mesa-sta...@lists.freedesktop.org
> ---
>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp 
> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> index af41bdb..6bf3c89 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> @@ -1633,7 +1633,7 @@ 
> glsl_to_tgsi_visitor::visit_expression(ir_expression* ir, st_src_reg *op)
>   emit_asm(ir, TGSI_OPCODE_DNEG, result_dst, op[0]);
>else {
>

Re: [Mesa-dev] [PATCH] st/glsl_to_tgsi: Translate float ir_unop_neg into float MOV with modifier.

2017-03-06 Thread Francisco Jerez 
Roland Scheidegger  writes:

> Am 04.03.2017 um 20:45 schrieb Ilia Mirkin:
>> Also, how is this happening in the first place? For example, we have:
>> 
>>case ir_unop_bitcast_f2i:
>>case ir_unop_bitcast_f2u:
>>   /* Make sure we don't propagate the negate modifier to integer 
>> opcodes. */
>>   if (op[0].negate || op[0].abs)
>>  emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
>>   else
>>  result_src = op[0];
>> 
>> Oh, but it's going directly into a ir_triop_csel, which is missing
>> this logic. It should be added there instead, IMHO. OTOH, the same
>> issue will hit in emit_block_mov() if you do. Would love to hear some
>> other opinions... Marek, Brian, Roland?
>
> float modifiers used with UCMP are actually just fine in theory - the
> UCMP sources are considered floats for the purposes of modifiers (this
> is similar to mov which also has untyped sources, except that 1 of the
> arguments of ucmp indeed is a uint, which cannot have modifiers).
> (tgsi_opcode_infer_type() says it's untyped, but
> tgsi_opcode_infer_src_type() says it's float though obviously this
> doesn't apply to the condition argument.)
>

Yikes...  That sounds terribly inconsistent...  Would it make more sense
to fix UCMP to behave like other integer instructions? (i.e. as the TGSI
docs claim it works)

> Therefore, not handling float modifiers with ucmp src args is a bug of
> the tgsi executor (I'm quite sure llvmpipe will handle it right). Looks
> like all source arguments must be the same type there... Seems annoying
> to fix actually...
> Albeit the gallium docs don't really mention this (this is the same
> behavior as dx10 movc). I don't know though if other drivers will handle
> it correctly, but if they do it might be a better idea to just fix
> tgsi_exec somehow.
>
> (I'm not sure if the opposite could happen, that is int modifiers
> mistakenly going into a ucmp, or if this could be a problem with other
> instructions.)
>
> Roland
>
>
>>   -ilia
>> 
>> 
>> On Sat, Mar 4, 2017 at 2:24 PM, Ilia Mirkin  wrote:
>>> Hmmm... I wonder if this should only be done for the native_integers
>>> case. I'm concerned that this will cause perf regressions on weaker hw
>>> like nv30 and r300, as the neg will no longer be inserted as a
>>> modifier into the next instruction. Any opinion on this?
>>>
>>> On Sat, Mar 4, 2017 at 2:16 PM, Francisco Jerez  
>>> wrote:
 Otherwise result_src may be provided to an integer instruction whose
 negate modifier has different semantics.  Example is UCMP as in the
 bug linked below, where an unrelated change in the GLSL built-in
 lowering code for atan2 (e9ffd12827ac11a2d2002a42fa8eb1df847153ba)
 caused the generation of floating-point ir_unop_neg instructions
 followed by ir_triop_csel, which is lowered into UCMP on back-ends
 with native integer support.

 Fixes a regression of piglit glsl-fs-tan-1 on softpipe introduced by
 the above-mentioned glsl front-end commit.  Even though the commit
 that triggered the regression doesn't seem to have made it to any
 stable branches yet, this seems worth back-porting since I don't see
 any reason why the bug couldn't have been reproduced before that
 point.

 Bugzilla: 
 https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_show-5Fbug.cgi-3Fid-3D99817=DwIGaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04=VpboBTuXbZaDL1-iEDED9JFLZTZFsPVqjTSbTuGSuT8=
  
 Tested-by: Vinson Lee 
 Cc: mesa-sta...@lists.freedesktop.org
 ---
  src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp 
 b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
 index af41bdb..6bf3c89 100644
 --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
 +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
 @@ -1633,7 +1633,7 @@ 
 glsl_to_tgsi_visitor::visit_expression(ir_expression* ir, st_src_reg *op)
   emit_asm(ir, TGSI_OPCODE_DNEG, result_dst, op[0]);
else {
   op[0].negate = ~op[0].negate;
 - result_src = op[0];
 + emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
}
break;
 case ir_unop_subroutine_to_int:
 --
 2.10.2

 ___
 mesa-dev mailing list
 mesa-dev@lists.freedesktop.org
 https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev=DwIGaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04=zkVQuaiGqon4ebXPnKm96V6UnqAVXfGc_ky9Lm_FMKY=
  
>> ___
>> mesa-dev mailing list
>>

Re: [Mesa-dev] [PATCH] st/glsl_to_tgsi: Translate float ir_unop_neg into float MOV with modifier.

2017-03-06 Thread Roland Scheidegger 
Am 06.03.2017 um 13:06 schrieb Roland Scheidegger:
> Am 04.03.2017 um 20:45 schrieb Ilia Mirkin:
>> Also, how is this happening in the first place? For example, we have:
>>
>>case ir_unop_bitcast_f2i:
>>case ir_unop_bitcast_f2u:
>>   /* Make sure we don't propagate the negate modifier to integer 
>> opcodes. */
>>   if (op[0].negate || op[0].abs)
>>  emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
>>   else
>>  result_src = op[0];
>>
>> Oh, but it's going directly into a ir_triop_csel, which is missing
>> this logic. It should be added there instead, IMHO. OTOH, the same
>> issue will hit in emit_block_mov() if you do. Would love to hear some
>> other opinions... Marek, Brian, Roland?
> 
> float modifiers used with UCMP are actually just fine in theory - the
> UCMP sources are considered floats for the purposes of modifiers (this
> is similar to mov which also has untyped sources, except that 1 of the
> arguments of ucmp indeed is a uint, which cannot have modifiers).
> (tgsi_opcode_infer_type() says it's untyped, but
> tgsi_opcode_infer_src_type() says it's float though obviously this
> doesn't apply to the condition argument.)
> 
> Therefore, not handling float modifiers with ucmp src args is a bug of
> the tgsi executor (I'm quite sure llvmpipe will handle it right). Looks
> like all source arguments must be the same type there... Seems annoying
> to fix actually...
On second thought, fixing this generically wouldn't really be required
(albeit possible too), since it just affects one opcode, just use some
special code instead of exec_vector_trinary.

Roland


> Albeit the gallium docs don't really mention this (this is the same
> behavior as dx10 movc). I don't know though if other drivers will handle
> it correctly, but if they do it might be a better idea to just fix
> tgsi_exec somehow.
> 
> (I'm not sure if the opposite could happen, that is int modifiers
> mistakenly going into a ucmp, or if this could be a problem with other
> instructions.)
> 
> Roland
> 
> 
>>   -ilia
>>
>>
>> On Sat, Mar 4, 2017 at 2:24 PM, Ilia Mirkin  wrote:
>>> Hmmm... I wonder if this should only be done for the native_integers
>>> case. I'm concerned that this will cause perf regressions on weaker hw
>>> like nv30 and r300, as the neg will no longer be inserted as a
>>> modifier into the next instruction. Any opinion on this?
>>>
>>> On Sat, Mar 4, 2017 at 2:16 PM, Francisco Jerez  
>>> wrote:
 Otherwise result_src may be provided to an integer instruction whose
 negate modifier has different semantics.  Example is UCMP as in the
 bug linked below, where an unrelated change in the GLSL built-in
 lowering code for atan2 (e9ffd12827ac11a2d2002a42fa8eb1df847153ba)
 caused the generation of floating-point ir_unop_neg instructions
 followed by ir_triop_csel, which is lowered into UCMP on back-ends
 with native integer support.

 Fixes a regression of piglit glsl-fs-tan-1 on softpipe introduced by
 the above-mentioned glsl front-end commit.  Even though the commit
 that triggered the regression doesn't seem to have made it to any
 stable branches yet, this seems worth back-porting since I don't see
 any reason why the bug couldn't have been reproduced before that
 point.

 Bugzilla: 
 https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_show-5Fbug.cgi-3Fid-3D99817=DwIGaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04=VpboBTuXbZaDL1-iEDED9JFLZTZFsPVqjTSbTuGSuT8=
  
 Tested-by: Vinson Lee 
 Cc: mesa-sta...@lists.freedesktop.org
 ---
  src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp 
 b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
 index af41bdb..6bf3c89 100644
 --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
 +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
 @@ -1633,7 +1633,7 @@ 
 glsl_to_tgsi_visitor::visit_expression(ir_expression* ir, st_src_reg *op)
   emit_asm(ir, TGSI_OPCODE_DNEG, result_dst, op[0]);
else {
   op[0].negate = ~op[0].negate;
 - result_src = op[0];
 + emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
}
break;
 case ir_unop_subroutine_to_int:
 --
 2.10.2

 ___
 mesa-dev mailing list
 mesa-dev@lists.freedesktop.org
 https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev=DwIGaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04=zkVQuaiGqon4ebXPnKm96V6UnqAVXfGc_ky9Lm_FMKY=
  
>> ___
>>

Re: [Mesa-dev] [PATCH] st/glsl_to_tgsi: Translate float ir_unop_neg into float MOV with modifier.

2017-03-06 Thread Marek Olšák 
I'm sure radeonsi will handle UCMP with modifiers too. (which is basically
gallivm)

Marek

On Mar 6, 2017 1:06 PM, "Roland Scheidegger"  wrote:

> Am 04.03.2017 um 20:45 schrieb Ilia Mirkin:
> > Also, how is this happening in the first place? For example, we have:
> >
> >case ir_unop_bitcast_f2i:
> >case ir_unop_bitcast_f2u:
> >   /* Make sure we don't propagate the negate modifier to integer
> opcodes. */
> >   if (op[0].negate || op[0].abs)
> >  emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
> >   else
> >  result_src = op[0];
> >
> > Oh, but it's going directly into a ir_triop_csel, which is missing
> > this logic. It should be added there instead, IMHO. OTOH, the same
> > issue will hit in emit_block_mov() if you do. Would love to hear some
> > other opinions... Marek, Brian, Roland?
>
> float modifiers used with UCMP are actually just fine in theory - the
> UCMP sources are considered floats for the purposes of modifiers (this
> is similar to mov which also has untyped sources, except that 1 of the
> arguments of ucmp indeed is a uint, which cannot have modifiers).
> (tgsi_opcode_infer_type() says it's untyped, but
> tgsi_opcode_infer_src_type() says it's float though obviously this
> doesn't apply to the condition argument.)
>
> Therefore, not handling float modifiers with ucmp src args is a bug of
> the tgsi executor (I'm quite sure llvmpipe will handle it right). Looks
> like all source arguments must be the same type there... Seems annoying
> to fix actually...
> Albeit the gallium docs don't really mention this (this is the same
> behavior as dx10 movc). I don't know though if other drivers will handle
> it correctly, but if they do it might be a better idea to just fix
> tgsi_exec somehow.
>
> (I'm not sure if the opposite could happen, that is int modifiers
> mistakenly going into a ucmp, or if this could be a problem with other
> instructions.)
>
> Roland
>
>
> >   -ilia
> >
> >
> > On Sat, Mar 4, 2017 at 2:24 PM, Ilia Mirkin 
> wrote:
> >> Hmmm... I wonder if this should only be done for the native_integers
> >> case. I'm concerned that this will cause perf regressions on weaker hw
> >> like nv30 and r300, as the neg will no longer be inserted as a
> >> modifier into the next instruction. Any opinion on this?
> >>
> >> On Sat, Mar 4, 2017 at 2:16 PM, Francisco Jerez 
> wrote:
> >>> Otherwise result_src may be provided to an integer instruction whose
> >>> negate modifier has different semantics.  Example is UCMP as in the
> >>> bug linked below, where an unrelated change in the GLSL built-in
> >>> lowering code for atan2 (e9ffd12827ac11a2d2002a42fa8eb1df847153ba)
> >>> caused the generation of floating-point ir_unop_neg instructions
> >>> followed by ir_triop_csel, which is lowered into UCMP on back-ends
> >>> with native integer support.
> >>>
> >>> Fixes a regression of piglit glsl-fs-tan-1 on softpipe introduced by
> >>> the above-mentioned glsl front-end commit.  Even though the commit
> >>> that triggered the regression doesn't seem to have made it to any
> >>> stable branches yet, this seems worth back-porting since I don't see
> >>> any reason why the bug couldn't have been reproduced before that
> >>> point.
> >>>
> >>> Bugzilla: https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.
> freedesktop.org_show-5Fbug.cgi-3Fid-3D99817=DwIGaQ=
> uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=
> ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04=VpboBTuXbZaDL1-
> iEDED9JFLZTZFsPVqjTSbTuGSuT8=
> >>> Tested-by: Vinson Lee 
> >>> Cc: mesa-sta...@lists.freedesktop.org
> >>> ---
> >>>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> >>> index af41bdb..6bf3c89 100644
> >>> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> >>> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> >>> @@ -1633,7 +1633,7 @@ 
> >>> glsl_to_tgsi_visitor::visit_expression(ir_expression*
> ir, st_src_reg *op)
> >>>   emit_asm(ir, TGSI_OPCODE_DNEG, result_dst, op[0]);
> >>>else {
> >>>   op[0].negate = ~op[0].negate;
> >>> - result_src = op[0];
> >>> + emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
> >>>}
> >>>break;
> >>> case ir_unop_subroutine_to_int:
> >>> --
> >>> 2.10.2
> >>>
> >>> ___
> >>> mesa-dev mailing list
> >>> mesa-dev@lists.freedesktop.org
> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.
> freedesktop.org_mailman_listinfo_mesa-2Ddev=DwIGaQ&
> c=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=
> ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04=
> zkVQuaiGqon4ebXPnKm96V6UnqAVXfGc_ky9Lm_FMKY=
> > ___
> > mesa-dev mailing

Re: [Mesa-dev] [PATCH] st/glsl_to_tgsi: Translate float ir_unop_neg into float MOV with modifier.

2017-03-06 Thread Roland Scheidegger 
Am 04.03.2017 um 20:45 schrieb Ilia Mirkin:
> Also, how is this happening in the first place? For example, we have:
> 
>case ir_unop_bitcast_f2i:
>case ir_unop_bitcast_f2u:
>   /* Make sure we don't propagate the negate modifier to integer opcodes. 
> */
>   if (op[0].negate || op[0].abs)
>  emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
>   else
>  result_src = op[0];
> 
> Oh, but it's going directly into a ir_triop_csel, which is missing
> this logic. It should be added there instead, IMHO. OTOH, the same
> issue will hit in emit_block_mov() if you do. Would love to hear some
> other opinions... Marek, Brian, Roland?

float modifiers used with UCMP are actually just fine in theory - the
UCMP sources are considered floats for the purposes of modifiers (this
is similar to mov which also has untyped sources, except that 1 of the
arguments of ucmp indeed is a uint, which cannot have modifiers).
(tgsi_opcode_infer_type() says it's untyped, but
tgsi_opcode_infer_src_type() says it's float though obviously this
doesn't apply to the condition argument.)

Therefore, not handling float modifiers with ucmp src args is a bug of
the tgsi executor (I'm quite sure llvmpipe will handle it right). Looks
like all source arguments must be the same type there... Seems annoying
to fix actually...
Albeit the gallium docs don't really mention this (this is the same
behavior as dx10 movc). I don't know though if other drivers will handle
it correctly, but if they do it might be a better idea to just fix
tgsi_exec somehow.

(I'm not sure if the opposite could happen, that is int modifiers
mistakenly going into a ucmp, or if this could be a problem with other
instructions.)

Roland


>   -ilia
> 
> 
> On Sat, Mar 4, 2017 at 2:24 PM, Ilia Mirkin  wrote:
>> Hmmm... I wonder if this should only be done for the native_integers
>> case. I'm concerned that this will cause perf regressions on weaker hw
>> like nv30 and r300, as the neg will no longer be inserted as a
>> modifier into the next instruction. Any opinion on this?
>>
>> On Sat, Mar 4, 2017 at 2:16 PM, Francisco Jerez  
>> wrote:
>>> Otherwise result_src may be provided to an integer instruction whose
>>> negate modifier has different semantics.  Example is UCMP as in the
>>> bug linked below, where an unrelated change in the GLSL built-in
>>> lowering code for atan2 (e9ffd12827ac11a2d2002a42fa8eb1df847153ba)
>>> caused the generation of floating-point ir_unop_neg instructions
>>> followed by ir_triop_csel, which is lowered into UCMP on back-ends
>>> with native integer support.
>>>
>>> Fixes a regression of piglit glsl-fs-tan-1 on softpipe introduced by
>>> the above-mentioned glsl front-end commit.  Even though the commit
>>> that triggered the regression doesn't seem to have made it to any
>>> stable branches yet, this seems worth back-porting since I don't see
>>> any reason why the bug couldn't have been reproduced before that
>>> point.
>>>
>>> Bugzilla: 
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_show-5Fbug.cgi-3Fid-3D99817=DwIGaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04=VpboBTuXbZaDL1-iEDED9JFLZTZFsPVqjTSbTuGSuT8=
>>>  
>>> Tested-by: Vinson Lee 
>>> Cc: mesa-sta...@lists.freedesktop.org
>>> ---
>>>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp 
>>> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>> index af41bdb..6bf3c89 100644
>>> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>> @@ -1633,7 +1633,7 @@ glsl_to_tgsi_visitor::visit_expression(ir_expression* 
>>> ir, st_src_reg *op)
>>>   emit_asm(ir, TGSI_OPCODE_DNEG, result_dst, op[0]);
>>>else {
>>>   op[0].negate = ~op[0].negate;
>>> - result_src = op[0];
>>> + emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
>>>}
>>>break;
>>> case ir_unop_subroutine_to_int:
>>> --
>>> 2.10.2
>>>
>>> ___
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev=DwIGaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04=zkVQuaiGqon4ebXPnKm96V6UnqAVXfGc_ky9Lm_FMKY=
>>>  
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev=DwIGaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04=zkVQuaiGqon4ebXPnKm96V6UnqAVXfGc_ky9Lm_FMKY=
>  
>

Re: [Mesa-dev] [PATCH] st/glsl_to_tgsi: Translate float ir_unop_neg into float MOV with modifier.

2017-03-04 Thread Ilia Mirkin 
On Sat, Mar 4, 2017 at 9:21 PM, Francisco Jerez  wrote:
> Ilia Mirkin  writes:
>
>> Also, how is this happening in the first place? For example, we have:
>>
>>case ir_unop_bitcast_f2i:
>>case ir_unop_bitcast_f2u:
>>   /* Make sure we don't propagate the negate modifier to integer 
>> opcodes. */
>>   if (op[0].negate || op[0].abs)
>>  emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
>>   else
>>  result_src = op[0];
>>
>> Oh, but it's going directly into a ir_triop_csel, which is missing
>> this logic. It should be added there instead, IMHO. OTOH, the same
>> issue will hit in emit_block_mov() if you do. Would love to hear some
>> other opinions... Marek, Brian, Roland?
>>
>
> I considered doing something like that but it will be somewhat more
> involved than in the snippet above because you'll have to allocate
> temporaries to hold the negated source results in case that any of the
> csel sources has a modifier set -- Can look into it next week if you
> think it's the right thing to do.

Right, that's mildly annoying but definitely solvable.

One last thought from me - for ir_unop_abs, we do the MOV. So perhaps
we should just suck it up and do the MOV here.

But I'd really like to hear from others.

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] st/glsl_to_tgsi: Translate float ir_unop_neg into float MOV with modifier.

2017-03-04 Thread Francisco Jerez 
Ilia Mirkin  writes:

> Also, how is this happening in the first place? For example, we have:
>
>case ir_unop_bitcast_f2i:
>case ir_unop_bitcast_f2u:
>   /* Make sure we don't propagate the negate modifier to integer opcodes. 
> */
>   if (op[0].negate || op[0].abs)
>  emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
>   else
>  result_src = op[0];
>
> Oh, but it's going directly into a ir_triop_csel, which is missing
> this logic. It should be added there instead, IMHO. OTOH, the same
> issue will hit in emit_block_mov() if you do. Would love to hear some
> other opinions... Marek, Brian, Roland?
>

I considered doing something like that but it will be somewhat more
involved than in the snippet above because you'll have to allocate
temporaries to hold the negated source results in case that any of the
csel sources has a modifier set -- Can look into it next week if you
think it's the right thing to do.

>   -ilia
>
>
> On Sat, Mar 4, 2017 at 2:24 PM, Ilia Mirkin  wrote:
>> Hmmm... I wonder if this should only be done for the native_integers
>> case. I'm concerned that this will cause perf regressions on weaker hw
>> like nv30 and r300, as the neg will no longer be inserted as a
>> modifier into the next instruction. Any opinion on this?
>>
>> On Sat, Mar 4, 2017 at 2:16 PM, Francisco Jerez  
>> wrote:
>>> Otherwise result_src may be provided to an integer instruction whose
>>> negate modifier has different semantics.  Example is UCMP as in the
>>> bug linked below, where an unrelated change in the GLSL built-in
>>> lowering code for atan2 (e9ffd12827ac11a2d2002a42fa8eb1df847153ba)
>>> caused the generation of floating-point ir_unop_neg instructions
>>> followed by ir_triop_csel, which is lowered into UCMP on back-ends
>>> with native integer support.
>>>
>>> Fixes a regression of piglit glsl-fs-tan-1 on softpipe introduced by
>>> the above-mentioned glsl front-end commit.  Even though the commit
>>> that triggered the regression doesn't seem to have made it to any
>>> stable branches yet, this seems worth back-porting since I don't see
>>> any reason why the bug couldn't have been reproduced before that
>>> point.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99817
>>> Tested-by: Vinson Lee 
>>> Cc: mesa-sta...@lists.freedesktop.org
>>> ---
>>>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp 
>>> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>> index af41bdb..6bf3c89 100644
>>> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>> @@ -1633,7 +1633,7 @@ glsl_to_tgsi_visitor::visit_expression(ir_expression* 
>>> ir, st_src_reg *op)
>>>   emit_asm(ir, TGSI_OPCODE_DNEG, result_dst, op[0]);
>>>else {
>>>   op[0].negate = ~op[0].negate;
>>> - result_src = op[0];
>>> + emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
>>>}
>>>break;
>>> case ir_unop_subroutine_to_int:
>>> --
>>> 2.10.2
>>>
>>> ___
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] st/glsl_to_tgsi: Translate float ir_unop_neg into float MOV with modifier.

2017-03-04 Thread Ilia Mirkin 
Also, how is this happening in the first place? For example, we have:

   case ir_unop_bitcast_f2i:
   case ir_unop_bitcast_f2u:
  /* Make sure we don't propagate the negate modifier to integer opcodes. */
  if (op[0].negate || op[0].abs)
 emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
  else
 result_src = op[0];

Oh, but it's going directly into a ir_triop_csel, which is missing
this logic. It should be added there instead, IMHO. OTOH, the same
issue will hit in emit_block_mov() if you do. Would love to hear some
other opinions... Marek, Brian, Roland?

  -ilia


On Sat, Mar 4, 2017 at 2:24 PM, Ilia Mirkin  wrote:
> Hmmm... I wonder if this should only be done for the native_integers
> case. I'm concerned that this will cause perf regressions on weaker hw
> like nv30 and r300, as the neg will no longer be inserted as a
> modifier into the next instruction. Any opinion on this?
>
> On Sat, Mar 4, 2017 at 2:16 PM, Francisco Jerez  wrote:
>> Otherwise result_src may be provided to an integer instruction whose
>> negate modifier has different semantics.  Example is UCMP as in the
>> bug linked below, where an unrelated change in the GLSL built-in
>> lowering code for atan2 (e9ffd12827ac11a2d2002a42fa8eb1df847153ba)
>> caused the generation of floating-point ir_unop_neg instructions
>> followed by ir_triop_csel, which is lowered into UCMP on back-ends
>> with native integer support.
>>
>> Fixes a regression of piglit glsl-fs-tan-1 on softpipe introduced by
>> the above-mentioned glsl front-end commit.  Even though the commit
>> that triggered the regression doesn't seem to have made it to any
>> stable branches yet, this seems worth back-porting since I don't see
>> any reason why the bug couldn't have been reproduced before that
>> point.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99817
>> Tested-by: Vinson Lee 
>> Cc: mesa-sta...@lists.freedesktop.org
>> ---
>>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp 
>> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>> index af41bdb..6bf3c89 100644
>> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>> @@ -1633,7 +1633,7 @@ glsl_to_tgsi_visitor::visit_expression(ir_expression* 
>> ir, st_src_reg *op)
>>   emit_asm(ir, TGSI_OPCODE_DNEG, result_dst, op[0]);
>>else {
>>   op[0].negate = ~op[0].negate;
>> - result_src = op[0];
>> + emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
>>}
>>break;
>> case ir_unop_subroutine_to_int:
>> --
>> 2.10.2
>>
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] st/glsl_to_tgsi: Translate float ir_unop_neg into float MOV with modifier.

2017-03-04 Thread Ilia Mirkin 
Hmmm... I wonder if this should only be done for the native_integers
case. I'm concerned that this will cause perf regressions on weaker hw
like nv30 and r300, as the neg will no longer be inserted as a
modifier into the next instruction. Any opinion on this?

On Sat, Mar 4, 2017 at 2:16 PM, Francisco Jerez  wrote:
> Otherwise result_src may be provided to an integer instruction whose
> negate modifier has different semantics.  Example is UCMP as in the
> bug linked below, where an unrelated change in the GLSL built-in
> lowering code for atan2 (e9ffd12827ac11a2d2002a42fa8eb1df847153ba)
> caused the generation of floating-point ir_unop_neg instructions
> followed by ir_triop_csel, which is lowered into UCMP on back-ends
> with native integer support.
>
> Fixes a regression of piglit glsl-fs-tan-1 on softpipe introduced by
> the above-mentioned glsl front-end commit.  Even though the commit
> that triggered the regression doesn't seem to have made it to any
> stable branches yet, this seems worth back-porting since I don't see
> any reason why the bug couldn't have been reproduced before that
> point.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99817
> Tested-by: Vinson Lee 
> Cc: mesa-sta...@lists.freedesktop.org
> ---
>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp 
> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> index af41bdb..6bf3c89 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> @@ -1633,7 +1633,7 @@ glsl_to_tgsi_visitor::visit_expression(ir_expression* 
> ir, st_src_reg *op)
>   emit_asm(ir, TGSI_OPCODE_DNEG, result_dst, op[0]);
>else {
>   op[0].negate = ~op[0].negate;
> - result_src = op[0];
> + emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
>}
>break;
> case ir_unop_subroutine_to_int:
> --
> 2.10.2
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] st/glsl_to_tgsi: Translate float ir_unop_neg into float MOV with modifier.

2017-03-04 Thread Francisco Jerez 
Otherwise result_src may be provided to an integer instruction whose
negate modifier has different semantics.  Example is UCMP as in the
bug linked below, where an unrelated change in the GLSL built-in
lowering code for atan2 (e9ffd12827ac11a2d2002a42fa8eb1df847153ba)
caused the generation of floating-point ir_unop_neg instructions
followed by ir_triop_csel, which is lowered into UCMP on back-ends
with native integer support.

Fixes a regression of piglit glsl-fs-tan-1 on softpipe introduced by
the above-mentioned glsl front-end commit.  Even though the commit
that triggered the regression doesn't seem to have made it to any
stable branches yet, this seems worth back-porting since I don't see
any reason why the bug couldn't have been reproduced before that
point.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99817
Tested-by: Vinson Lee 
Cc: mesa-sta...@lists.freedesktop.org
---
 src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp 
b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
index af41bdb..6bf3c89 100644
--- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
+++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
@@ -1633,7 +1633,7 @@ glsl_to_tgsi_visitor::visit_expression(ir_expression* ir, 
st_src_reg *op)
  emit_asm(ir, TGSI_OPCODE_DNEG, result_dst, op[0]);
   else {
  op[0].negate = ~op[0].negate;
- result_src = op[0];
+ emit_asm(ir, TGSI_OPCODE_MOV, result_dst, op[0]);
   }
   break;
case ir_unop_subroutine_to_int:
-- 
2.10.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev