Re: [Mesa-dev] [PATCH] glsl: Fix function return typechecking
Hi,
On Thu, Feb 21, 2019 at 10:35 AM Tapani Pälli
wrote:
> Hi;
>
> On 2/11/19 6:46 PM, Oscar Blumberg wrote:
> > apply_implicit_conversion only converts and check base types but we
> > need actual type equality for function returns, otherwise you can
> > return a vec2 from a function declared as returning a float.
>
> Do you have some test shader that hits this condition? It seems to me
> that currently we will error out correctly if one tries to return vec2
> from function declared as returning a float.
>
>
Yes, I believe it triggers in even the simplest case here, such as :
float f() { return vec2(1.0); }
anywhere in the shader.
Does it fail to reproduce on your end ?
(note that the declared glsl version must be recent enough that implicit
conversion for function returns are enabled, I'm using 450 here).
I believe most of the confusion comes from the name of the
apply_implicit_conversion function, since it is mainly used for arithmetic
operations for which, e.g., vec2+float is allowed.
Because of that it only checks (and convert in place) base types without
looking at element count for vector-like things.
We can still use it to perform the conversion but it requires an additional
check, hence the patch.
That's my understanding of it anyway.
> ---
> > src/compiler/glsl/ast_to_hir.cpp | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/compiler/glsl/ast_to_hir.cpp
> b/src/compiler/glsl/ast_to_hir.cpp
> > index 620153e6a34..6bf2910954f 100644
> > --- a/src/compiler/glsl/ast_to_hir.cpp
> > +++ b/src/compiler/glsl/ast_to_hir.cpp
> > @@ -6248,7 +6248,8 @@ ast_jump_statement::hir(exec_list *instructions,
> >
> > if (state->has_420pack()) {
> > if
> (!apply_implicit_conversion(state->current_function->return_type,
> > - ret, state)) {
> > + ret, state)
> > + || (ret->type !=
> state->current_function->return_type)) {
> > _mesa_glsl_error(& loc, state,
> > "could not implicitly convert
> return value "
> > "to %s, in function `%s'",
> >
> ___
> 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
[Mesa-dev] [PATCH] radeonsi: Fix guardband computation for large render targets
Stop using 12.12 quantization for viewports that are not contained in
the lower 4k corner of the render target as the hardware needs to keep
both absolute and relative coordinates representable.
---
.../drivers/radeonsi/si_state_viewport.c | 30 +--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/src/gallium/drivers/radeonsi/si_state_viewport.c
b/src/gallium/drivers/radeonsi/si_state_viewport.c
index dac90df1c4f..64bb956b200 100644
--- a/src/gallium/drivers/radeonsi/si_state_viewport.c
+++ b/src/gallium/drivers/radeonsi/si_state_viewport.c
@@ -185,6 +185,16 @@ static void si_emit_guardband(struct si_context *ctx)
const unsigned hw_screen_offset_alignment =
ctx->chip_class >= VI ? 16 : MAX2(ctx->screen->se_tile_repeat,
16);
+ /* Indexed by quantization modes */
+ static unsigned max_viewport_size[] = {65535, 16383, 4095};
+
+ /* Ensure that the whole viewport stays representable in
+* absolute coordinates.
+* See comment in si_set_viewport_states.
+*/
+ assert(vp_as_scissor.maxx <=
max_viewport_size[vp_as_scissor.quant_mode] &&
+ vp_as_scissor.maxy <=
max_viewport_size[vp_as_scissor.quant_mode]);
+
hw_screen_offset_x = CLAMP(hw_screen_offset_x, 0,
MAX_PA_SU_HARDWARE_SCREEN_OFFSET);
hw_screen_offset_y = CLAMP(hw_screen_offset_y, 0,
MAX_PA_SU_HARDWARE_SCREEN_OFFSET);
@@ -219,7 +229,6 @@ static void si_emit_guardband(struct si_context *ctx)
*
* The viewport range is [-max_viewport_size/2, max_viewport_size/2].
*/
- static unsigned max_viewport_size[] = {65535, 16383, 4095};
assert(vp_as_scissor.quant_mode < ARRAY_SIZE(max_viewport_size));
max_range = max_viewport_size[vp_as_scissor.quant_mode] / 2;
left = (-max_range - vp.translate[0]) / vp.scale[0];
@@ -333,6 +342,8 @@ static void si_set_viewport_states(struct pipe_context
*pctx,
unsigned h = scissor->maxy - scissor->miny;
unsigned max_extent = MAX2(w, h);
+ int max_corner = MAX2(scissor->maxx, scissor->maxy);
+
unsigned center_x = (scissor->maxx + scissor->minx) / 2;
unsigned center_y = (scissor->maxy + scissor->miny) / 2;
unsigned max_center = MAX2(center_x, center_y);
@@ -358,7 +369,22 @@ static void si_set_viewport_states(struct pipe_context
*pctx,
if (ctx->family == CHIP_RAVEN)
max_extent = 16384; /* Use QUANT_MODE == 16_8. */
- if (max_extent <= 1024) /* 4K scanline area for guardband */
+ /* Another constraint is that all coordinates in the viewport
+* are representable in fixed point with respect to the
+* surface origin.
+*
+* It means that PA_SU_HARDWARE_SCREEN_OFFSET can't be given
+* an offset that would make the upper corner of the viewport
+* greater than the maximum representable number post
+* quantization, ie 2^quant_bits.
+*
+* This does not matter for 14.10 and 16.8 formats since the
+* offset is already limited at 8k, but it means we can't use
+* 12.12 if we are drawing to some pixels outside the lower
+* 4k x 4k of the render target.
+*/
+
+ if (max_extent <= 1024 && max_corner < 4096) /* 4K scanline
area for guardband */
scissor->quant_mode =
SI_QUANT_MODE_12_12_FIXED_POINT_1_4096TH;
else if (max_extent <= 4096) /* 16K scanline area for guardband
*/
scissor->quant_mode =
SI_QUANT_MODE_14_10_FIXED_POINT_1_1024TH;
--
2.20.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] glsl: Fix function return typechecking
apply_implicit_conversion only converts and check base types but we
need actual type equality for function returns, otherwise you can
return a vec2 from a function declared as returning a float.
---
src/compiler/glsl/ast_to_hir.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
index 620153e6a34..6bf2910954f 100644
--- a/src/compiler/glsl/ast_to_hir.cpp
+++ b/src/compiler/glsl/ast_to_hir.cpp
@@ -6248,7 +6248,8 @@ ast_jump_statement::hir(exec_list *instructions,
if (state->has_420pack()) {
if
(!apply_implicit_conversion(state->current_function->return_type,
- ret, state)) {
+ ret, state)
+ || (ret->type != state->current_function->return_type)) {
_mesa_glsl_error(& loc, state,
"could not implicitly convert return value "
"to %s, in function `%s'",
--
2.20.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] intel/fs: Fix memory corruption when compiling a CS
Missing check for shader stage in the fs_visitor would corrupt the cs_prog_data.push information and trigger crashes / corruption later when uploading the CS state. --- src/intel/compiler/brw_fs_nir.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/intel/compiler/brw_fs_nir.cpp b/src/intel/compiler/brw_fs_nir.cpp index bdc883e53..21b03a089 100644 --- a/src/intel/compiler/brw_fs_nir.cpp +++ b/src/intel/compiler/brw_fs_nir.cpp @@ -3779,8 +3779,9 @@ fs_visitor::nir_emit_intrinsic(const fs_builder , nir_intrinsic_instr *instr BRW_REGISTER_TYPE_UD); const fs_reg data = retype(get_nir_src(instr->src[2]), BRW_REGISTER_TYPE_UD); - - brw_wm_prog_data(prog_data)->has_side_effects = true; + + if (stage == MESA_SHADER_FRAGMENT) + brw_wm_prog_data(prog_data)->has_side_effects = true; emit_untyped_write(bld, image, addr, data, 1, instr->num_components); -- 2.20.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
