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

Mon Mar 6 12:30:53 UTC 2017

I'm sure radeonsi will handle UCMP with modifiers too. (which is basically
gallivm)

Marek

On Mar 6, 2017 1:06 PM, "Roland Scheidegger" <sroland at 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 <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=
> >
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170306/dc6054fb/attachment-0001.html>