Re: [Mesa-dev] [PATCH 1/9] i965/vec4: Don't emit MOVs for unused URB slots.
On Wed, Oct 21, 2015 at 1:52 PM, Emil Velikov wrote: > On 20 October 2015 at 05:08, Matt Turner wrote: >> Otherwise we'd emit a MOV from the null register (which isn't allowed). >> > Would you say it's a good idea to push the check down to the MOV() > implementation ? If not perhaps we should add an assert() to easily > catch cases like these in the future ? Sort of, yes. :) This series arose by writing an assembly validator that I plan to send today. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/9] i965/vec4: Don't emit MOVs for unused URB slots.
On 20 October 2015 at 05:08, Matt Turner wrote: > Otherwise we'd emit a MOV from the null register (which isn't allowed). > Would you say it's a good idea to push the check down to the MOV() implementation ? If not perhaps we should add an assert() to easily catch cases like these in the future ? Thanks Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/9] i965/vec4: Don't emit MOVs for unused URB slots.
On Tue, Oct 20, 2015 at 1:01 AM, Iago Toral wrote: > On Mon, 2015-10-19 at 21:08 -0700, Matt Turner wrote: >> Otherwise we'd emit a MOV from the null register (which isn't allowed). >> >> Helps 24 programs in shader-db (the geometry shaders in GSCloth): >> >> instructions in affected programs: 302 -> 262 (-13.25%) > > Makes sense to me, > > Reviewed-by: Iago Toral Quiroga > > Out of curiosity, does moving from the null register have any actual > consequences? The documentation says that it's not allowed, and I *think* I've seen a case where it caused a GPU hang but I cannot remember for sure. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/9] i965/vec4: Don't emit MOVs for unused URB slots.
On Mon, 2015-10-19 at 21:08 -0700, Matt Turner wrote:
> Otherwise we'd emit a MOV from the null register (which isn't allowed).
>
> Helps 24 programs in shader-db (the geometry shaders in GSCloth):
>
> instructions in affected programs: 302 -> 262 (-13.25%)
Makes sense to me,
Reviewed-by: Iago Toral Quiroga
Out of curiosity, does moving from the null register have any actual
consequences?
Iago
> ---
> src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp| 18 +-
> src/mesa/drivers/dri/i965/brw_vec4_vs_visitor.cpp | 2 +-
> 2 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> index f891910..c39f97e 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> @@ -1221,6 +1221,9 @@ vec4_visitor::emit_untyped_surface_read(unsigned
> surf_index, dst_reg dst,
> void
> vec4_visitor::emit_ndc_computation()
> {
> + if (output_reg[VARYING_SLOT_POS].file == BAD_FILE)
> + return;
> +
> /* Get the position */
> src_reg pos = src_reg(output_reg[VARYING_SLOT_POS]);
>
> @@ -1286,7 +1289,8 @@ vec4_visitor::emit_psiz_and_flags(dst_reg reg)
> * Later, clipping will detect ucp[6] and ensure the primitive is
> * clipped against all fixed planes.
> */
> - if (devinfo->has_negative_rhw_bug) {
> + if (devinfo->has_negative_rhw_bug &&
> + output_reg[BRW_VARYING_SLOT_NDC].file != BAD_FILE) {
> src_reg ndc_w = src_reg(output_reg[BRW_VARYING_SLOT_NDC]);
> ndc_w.swizzle = BRW_SWIZZLE_;
> emit(CMP(dst_null_f(), ndc_w, src_reg(0.0f), BRW_CONDITIONAL_L));
> @@ -1334,8 +1338,10 @@ vec4_visitor::emit_generic_urb_slot(dst_reg reg, int
> varying)
> assert(varying < VARYING_SLOT_MAX);
> assert(output_reg[varying].type == reg.type);
> current_annotation = output_reg_annotation[varying];
> - /* Copy the register, saturating if necessary */
> - return emit(MOV(reg, src_reg(output_reg[varying])));
> + if (output_reg[varying].file != BAD_FILE)
> + return emit(MOV(reg, src_reg(output_reg[varying])));
> + else
> + return NULL;
> }
>
> void
> @@ -1354,11 +1360,13 @@ vec4_visitor::emit_urb_slot(dst_reg reg, int varying)
> }
> case BRW_VARYING_SLOT_NDC:
>current_annotation = "NDC";
> - emit(MOV(reg, src_reg(output_reg[BRW_VARYING_SLOT_NDC])));
> + if (output_reg[BRW_VARYING_SLOT_NDC].file != BAD_FILE)
> + emit(MOV(reg, src_reg(output_reg[BRW_VARYING_SLOT_NDC])));
>break;
> case VARYING_SLOT_POS:
>current_annotation = "gl_Position";
> - emit(MOV(reg, src_reg(output_reg[VARYING_SLOT_POS])));
> + if (output_reg[VARYING_SLOT_POS].file != BAD_FILE)
> + emit(MOV(reg, src_reg(output_reg[VARYING_SLOT_POS])));
>break;
> case VARYING_SLOT_EDGE:
>/* This is present when doing unfilled polygons. We're supposed to
> copy
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_vs_visitor.cpp
> b/src/mesa/drivers/dri/i965/brw_vec4_vs_visitor.cpp
> index 485a80e..5dd4f98 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_vs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_vs_visitor.cpp
> @@ -217,7 +217,7 @@ vec4_vs_visitor::emit_urb_slot(dst_reg reg, int varying)
> * shader.
> */
>vec4_instruction *inst = emit_generic_urb_slot(reg, varying);
> - if (key->clamp_vertex_color)
> + if (inst && key->clamp_vertex_color)
> inst->saturate = true;
>break;
> }
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/9] i965/vec4: Don't emit MOVs for unused URB slots.
Otherwise we'd emit a MOV from the null register (which isn't allowed).
Helps 24 programs in shader-db (the geometry shaders in GSCloth):
instructions in affected programs: 302 -> 262 (-13.25%)
---
src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp| 18 +-
src/mesa/drivers/dri/i965/brw_vec4_vs_visitor.cpp | 2 +-
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
index f891910..c39f97e 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
@@ -1221,6 +1221,9 @@ vec4_visitor::emit_untyped_surface_read(unsigned
surf_index, dst_reg dst,
void
vec4_visitor::emit_ndc_computation()
{
+ if (output_reg[VARYING_SLOT_POS].file == BAD_FILE)
+ return;
+
/* Get the position */
src_reg pos = src_reg(output_reg[VARYING_SLOT_POS]);
@@ -1286,7 +1289,8 @@ vec4_visitor::emit_psiz_and_flags(dst_reg reg)
* Later, clipping will detect ucp[6] and ensure the primitive is
* clipped against all fixed planes.
*/
- if (devinfo->has_negative_rhw_bug) {
+ if (devinfo->has_negative_rhw_bug &&
+ output_reg[BRW_VARYING_SLOT_NDC].file != BAD_FILE) {
src_reg ndc_w = src_reg(output_reg[BRW_VARYING_SLOT_NDC]);
ndc_w.swizzle = BRW_SWIZZLE_;
emit(CMP(dst_null_f(), ndc_w, src_reg(0.0f), BRW_CONDITIONAL_L));
@@ -1334,8 +1338,10 @@ vec4_visitor::emit_generic_urb_slot(dst_reg reg, int
varying)
assert(varying < VARYING_SLOT_MAX);
assert(output_reg[varying].type == reg.type);
current_annotation = output_reg_annotation[varying];
- /* Copy the register, saturating if necessary */
- return emit(MOV(reg, src_reg(output_reg[varying])));
+ if (output_reg[varying].file != BAD_FILE)
+ return emit(MOV(reg, src_reg(output_reg[varying])));
+ else
+ return NULL;
}
void
@@ -1354,11 +1360,13 @@ vec4_visitor::emit_urb_slot(dst_reg reg, int varying)
}
case BRW_VARYING_SLOT_NDC:
current_annotation = "NDC";
- emit(MOV(reg, src_reg(output_reg[BRW_VARYING_SLOT_NDC])));
+ if (output_reg[BRW_VARYING_SLOT_NDC].file != BAD_FILE)
+ emit(MOV(reg, src_reg(output_reg[BRW_VARYING_SLOT_NDC])));
break;
case VARYING_SLOT_POS:
current_annotation = "gl_Position";
- emit(MOV(reg, src_reg(output_reg[VARYING_SLOT_POS])));
+ if (output_reg[VARYING_SLOT_POS].file != BAD_FILE)
+ emit(MOV(reg, src_reg(output_reg[VARYING_SLOT_POS])));
break;
case VARYING_SLOT_EDGE:
/* This is present when doing unfilled polygons. We're supposed to copy
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_vs_visitor.cpp
b/src/mesa/drivers/dri/i965/brw_vec4_vs_visitor.cpp
index 485a80e..5dd4f98 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_vs_visitor.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_vs_visitor.cpp
@@ -217,7 +217,7 @@ vec4_vs_visitor::emit_urb_slot(dst_reg reg, int varying)
* shader.
*/
vec4_instruction *inst = emit_generic_urb_slot(reg, varying);
- if (key->clamp_vertex_color)
+ if (inst && key->clamp_vertex_color)
inst->saturate = true;
break;
}
--
2.4.9
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
