I'm sure radeonsi will handle UCMP with modifiers too. (which is basically gallivm)
Marek On Mar 6, 2017 1:06 PM, "Roland Scheidegger" <srol...@vmware.com> 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 <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= > > > > _______________________________________________ > 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
