Roland Scheidegger <srol...@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 <imir...@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 <curroje...@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 <v...@freedesktop.org> >>>> 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&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04&s=zkVQuaiGqon4ebXPnKm96V6UnqAVXfGc_ky9Lm_FMKY&e= >>>> >> _______________________________________________ >> 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&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=ak9IRpzPmkscMdh984JOu0Zn2I-efj9Iy3K-Iw9vv04&s=zkVQuaiGqon4ebXPnKm96V6UnqAVXfGc_ky9Lm_FMKY&e= >> >>
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev