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

[Francisco Jerez]

Roland Scheidegger <sroland at vmware.com> 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 <imirkin at alum.mit.edu> 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 <currojerez at riseup.net> 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&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04&s=VpboBTuXbZaDL1-iEDED9JFLZTZFsPVqjTSbTuGSuT8&e= 
>>>> Tested-by: Vinson Lee <vlee at freedesktop.org>
>>>> Cc: mesa-stable at 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 at lists.freedesktop.org
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04&s=zkVQuaiGqon4ebXPnKm96V6UnqAVXfGc_ky9Lm_FMKY&e= 
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04&s=zkVQuaiGqon4ebXPnKm96V6UnqAVXfGc_ky9Lm_FMKY&e= 
>> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170306/871de721/attachment.sig>