[Mesa-dev] [PATCH 2/4] llvmpipe: enable clear_texture with util_clear_texture
Passes all corresponding piglit tests. Signed-off-by: Lars Hamre <cheme...@gmail.com> --- CC: Roland Scheidegger <srol...@vmware.com> NOTE: someone with access will need to commit this post review process src/gallium/drivers/llvmpipe/lp_screen.c | 3 ++- src/gallium/drivers/llvmpipe/lp_surface.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/gallium/drivers/llvmpipe/lp_screen.c b/src/gallium/drivers/llvmpipe/lp_screen.c index 76a30a6..2633b0c 100644 --- a/src/gallium/drivers/llvmpipe/lp_screen.c +++ b/src/gallium/drivers/llvmpipe/lp_screen.c @@ -307,6 +307,8 @@ llvmpipe_get_param(struct pipe_screen *screen, enum pipe_cap param) return 1; case PIPE_CAP_COPY_BETWEEN_COMPRESSED_AND_PLAIN_FORMATS: return 1; + case PIPE_CAP_CLEAR_TEXTURE: + return 1; case PIPE_CAP_MULTISAMPLE_Z_RESOLVE: case PIPE_CAP_RESOURCE_FROM_USER_MEMORY: case PIPE_CAP_DEVICE_RESET_STATUS_QUERY: @@ -315,7 +317,6 @@ llvmpipe_get_param(struct pipe_screen *screen, enum pipe_cap param) case PIPE_CAP_TGSI_TXQS: case PIPE_CAP_FORCE_PERSAMPLE_INTERP: case PIPE_CAP_SHAREABLE_SHADERS: - case PIPE_CAP_CLEAR_TEXTURE: case PIPE_CAP_DRAW_PARAMETERS: case PIPE_CAP_TGSI_PACK_HALF_FLOAT: case PIPE_CAP_MULTI_DRAW_INDIRECT: diff --git a/src/gallium/drivers/llvmpipe/lp_surface.c b/src/gallium/drivers/llvmpipe/lp_surface.c index 784db7f..953b26e 100644 --- a/src/gallium/drivers/llvmpipe/lp_surface.c +++ b/src/gallium/drivers/llvmpipe/lp_surface.c @@ -231,7 +231,8 @@ llvmpipe_init_surface_functions(struct llvmpipe_context *lp) lp->pipe.clear_depth_stencil = llvmpipe_clear_depth_stencil; lp->pipe.create_surface = llvmpipe_create_surface; lp->pipe.surface_destroy = llvmpipe_surface_destroy; - /* These two are not actually functions dealing with surfaces */ + /* These are not actually functions dealing with surfaces */ + lp->pipe.clear_texture = util_clear_texture; lp->pipe.resource_copy_region = lp_resource_copy; lp->pipe.blit = lp_blit; lp->pipe.flush_resource = lp_flush_resource; -- 2.7.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/4 v3] gallium: implement util_clear_texture
v3: have util_clear_texture mirror the pipe function (Roland Scheidegger)
v2: rework util clear functions such that they operate on a resource
instead of a surface (Roland Scheidegger)
Creates a util_clear_texture function for implementing the GL_ARB_clear_texture
in softpipe and llvmpipe.
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
CC: Roland Scheidegger <srol...@vmware.com>
NOTE: someone with access will need to commit this post
review process
src/gallium/auxiliary/util/u_surface.c | 386 -
src/gallium/auxiliary/util/u_surface.h | 7 +
2 files changed, 248 insertions(+), 145 deletions(-)
diff --git a/src/gallium/auxiliary/util/u_surface.c
b/src/gallium/auxiliary/util/u_surface.c
index a9ed006..f2a471d 100644
--- a/src/gallium/auxiliary/util/u_surface.c
+++ b/src/gallium/auxiliary/util/u_surface.c
@@ -388,6 +388,66 @@ no_src_map:
;
}
+static void
+util_clear_color_texture_helper(struct pipe_transfer *dst_trans,
+ubyte *dst_map,
+enum pipe_format format,
+const union pipe_color_union *color,
+unsigned width, unsigned height, unsigned
depth)
+{
+ union util_color uc;
+
+ assert(dst_trans->stride > 0);
+
+ if (util_format_is_pure_integer(format)) {
+ /*
+ * We expect int/uint clear values here, though some APIs
+ * might disagree (but in any case util_pack_color()
+ * couldn't handle it)...
+ */
+ if (util_format_is_pure_sint(format)) {
+ util_format_write_4i(format, color->i, 0, , 0, 0, 0, 1, 1);
+ } else {
+ assert(util_format_is_pure_uint(format));
+ util_format_write_4ui(format, color->ui, 0, , 0, 0, 0, 1, 1);
+ }
+ } else {
+ util_pack_color(color->f, format, );
+ }
+
+ util_fill_box(dst_map, format,
+ dst_trans->stride, dst_trans->layer_stride,
+ 0, 0, 0, width, height, depth, );
+}
+
+static void
+util_clear_color_texture(struct pipe_context *pipe,
+ struct pipe_resource *texture,
+ const union pipe_color_union *color,
+ unsigned level,
+ unsigned dstx, unsigned dsty, unsigned dstz,
+ unsigned width, unsigned height, unsigned depth)
+{
+ struct pipe_transfer *dst_trans;
+ ubyte *dst_map;
+ enum pipe_format format = texture->format;
+
+ dst_map = pipe_transfer_map_3d(pipe,
+ texture,
+ level,
+ PIPE_TRANSFER_WRITE,
+ dstx, dsty, dstz,
+ width, height, depth,
+ _trans);
+ if (!dst_map)
+ return;
+
+ if (dst_trans->stride > 0) {
+ util_clear_color_texture_helper(dst_trans, dst_map, format, color,
+ width, height, depth);
+ }
+ pipe->transfer_unmap(pipe, dst_trans);
+}
#define UBYTE_TO_USHORT(B) ((B) | ((B) << 8))
@@ -410,8 +470,6 @@ util_clear_render_target(struct pipe_context *pipe,
{
struct pipe_transfer *dst_trans;
ubyte *dst_map;
- union util_color uc;
- unsigned max_layer;
assert(dst->texture);
if (!dst->texture)
@@ -426,56 +484,202 @@ util_clear_render_target(struct pipe_context *pipe,
unsigned pixstride = util_format_get_blocksize(dst->format);
dx = (dst->u.buf.first_element + dstx) * pixstride;
w = width * pixstride;
- max_layer = 0;
dst_map = pipe_transfer_map(pipe,
dst->texture,
0, 0,
PIPE_TRANSFER_WRITE,
dx, 0, w, 1,
_trans);
+ if (dst_map) {
+ util_clear_color_texture_helper(dst_trans, dst_map, dst->format,
color,
+ width, height, 1);
+ pipe->transfer_unmap(pipe, dst_trans);
+ }
}
else {
- max_layer = dst->u.tex.last_layer - dst->u.tex.first_layer;
- dst_map = pipe_transfer_map_3d(pipe,
- dst->texture,
- dst->u.tex.level,
- PIPE_TRANSFER_WRITE,
- dstx, dsty, dst->u.tex.first_layer,
- width, height, max_layer + 1, _trans);
+ unsigned depth = dst->u.tex.last_layer - dst->u.tex.first_layer + 1;
+ util_clear_color_texture(pipe, dst->texture, color, dst->u.tex.level,
+ dstx, dsty, dst->u.tex.first_layer,
+ width, height, depth);
}
+}
+static void
[Mesa-dev] [PATCH 3/4] softpipe: enable clear_texture with util_clear_texture
Passes all corresponding piglit tests. Signed-off-by: Lars Hamre <cheme...@gmail.com> --- CC: Roland Scheidegger <srol...@vmware.com> NOTE: someone with access will need to commit this post review process src/gallium/drivers/softpipe/sp_screen.c | 3 ++- src/gallium/drivers/softpipe/sp_texture.c | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/gallium/drivers/softpipe/sp_screen.c b/src/gallium/drivers/softpipe/sp_screen.c index 02eff91..aa061d7 100644 --- a/src/gallium/drivers/softpipe/sp_screen.c +++ b/src/gallium/drivers/softpipe/sp_screen.c @@ -260,6 +260,8 @@ softpipe_get_param(struct pipe_screen *screen, enum pipe_cap param) case PIPE_CAP_COPY_BETWEEN_COMPRESSED_AND_PLAIN_FORMATS: case PIPE_CAP_TGSI_ARRAY_COMPONENTS: return 1; + case PIPE_CAP_CLEAR_TEXTURE: + return 1; case PIPE_CAP_MULTISAMPLE_Z_RESOLVE: case PIPE_CAP_RESOURCE_FROM_USER_MEMORY: case PIPE_CAP_DEVICE_RESET_STATUS_QUERY: @@ -268,7 +270,6 @@ softpipe_get_param(struct pipe_screen *screen, enum pipe_cap param) case PIPE_CAP_TGSI_TXQS: case PIPE_CAP_FORCE_PERSAMPLE_INTERP: case PIPE_CAP_SHAREABLE_SHADERS: - case PIPE_CAP_CLEAR_TEXTURE: case PIPE_CAP_DRAW_PARAMETERS: case PIPE_CAP_TGSI_PACK_HALF_FLOAT: case PIPE_CAP_MULTI_DRAW_INDIRECT: diff --git a/src/gallium/drivers/softpipe/sp_texture.c b/src/gallium/drivers/softpipe/sp_texture.c index 8dca158..ea5e2c6 100644 --- a/src/gallium/drivers/softpipe/sp_texture.c +++ b/src/gallium/drivers/softpipe/sp_texture.c @@ -37,6 +37,7 @@ #include "util/u_math.h" #include "util/u_memory.h" #include "util/u_transfer.h" +#include "util/u_surface.h" #include "sp_context.h" #include "sp_flush.h" @@ -520,6 +521,7 @@ softpipe_init_texture_funcs(struct pipe_context *pipe) pipe->create_surface = softpipe_create_surface; pipe->surface_destroy = softpipe_surface_destroy; + pipe->clear_texture = util_clear_texture; } -- 2.7.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 4/4] docs: update features.txt for GL_ARB_clear_texture with llvmpipe and softpipe
Signed-off-by: Lars Hamre <cheme...@gmail.com> --- CC: Roland Scheidegger <srol...@vmware.com> NOTE: someone with access will need to commit this post review process docs/features.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/features.txt b/docs/features.txt index 01315a0..d7828b1 100644 --- a/docs/features.txt +++ b/docs/features.txt @@ -192,7 +192,7 @@ GL 4.4, GLSL 4.40 -- all DONE: i965/gen8+, nvc0, radeonsi GL_MAX_VERTEX_ATTRIB_STRIDE DONE (all drivers) GL_ARB_buffer_storage DONE (i965, nv50, r600) - GL_ARB_clear_texture DONE (i965, nv50, r600) + GL_ARB_clear_texture DONE (i965, nv50, r600, llvmpipe, softpipe) GL_ARB_enhanced_layouts DONE (i965, nv50, llvmpipe, softpipe) - compile-time constant expressions DONE - explicit byte offsets for blocksDONE -- 2.7.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] softpipe: implement clear_texture
That does seem nicer, then pipe->clear_texture could just be set to
util_clear_texture.
Tangentially, do you see anything preventing this solution being
utilized by llvmpipe?
On Mon, Feb 20, 2017 at 2:31 PM, Roland Scheidegger <srol...@vmware.com> wrote:
> Am 20.02.2017 um 18:01 schrieb Lars Hamre:
>> v2: rework util clear functions such that they operate on a resource
>> instead of a surface (Roland Scheidegger)
>>
>> Implements the ARB_clear_texture extension for softpipe.
>> Passes all corresponding piglit tests.
>>
>> Signed-off-by: Lars Hamre <cheme...@gmail.com>
>>
>> ---
>>
>> CC: Roland Scheidegger <srol...@vmware.com>
>>
>> NOTE: someone with access will need to commit this post
>> review process
>>
>> src/gallium/auxiliary/util/u_surface.c| 339
>> +-
>> src/gallium/auxiliary/util/u_surface.h| 17 ++
>> src/gallium/drivers/softpipe/sp_screen.c | 3 +-
>> src/gallium/drivers/softpipe/sp_texture.c | 51 +
>> 4 files changed, 262 insertions(+), 148 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/util/u_surface.c
>> b/src/gallium/auxiliary/util/u_surface.c
>> index a9ed006..1ec168f 100644
>> --- a/src/gallium/auxiliary/util/u_surface.c
>> +++ b/src/gallium/auxiliary/util/u_surface.c
>> @@ -388,7 +388,66 @@ no_src_map:
>> ;
>> }
>>
>> +static void
>> +util_clear_texture_helper(struct pipe_transfer *dst_trans,
>> + ubyte *dst_map,
>> + enum pipe_format format,
>> + const union pipe_color_union *color,
>> + unsigned width, unsigned height, unsigned depth)
>> +{
>> + union util_color uc;
>> +
>> + assert(dst_trans->stride > 0);
>> +
>> + if (util_format_is_pure_integer(format)) {
>> + /*
>> + * We expect int/uint clear values here, though some APIs
>> + * might disagree (but in any case util_pack_color()
>> + * couldn't handle it)...
>> + */
>> + if (util_format_is_pure_sint(format)) {
>> + util_format_write_4i(format, color->i, 0, , 0, 0, 0, 1, 1);
>> + } else {
>> + assert(util_format_is_pure_uint(format));
>> + util_format_write_4ui(format, color->ui, 0, , 0, 0, 0, 1, 1);
>> + }
>> + } else {
>> + util_pack_color(color->f, format, );
>> + }
>> +
>> + util_fill_box(dst_map, format,
>> + dst_trans->stride, dst_trans->layer_stride,
>> + 0, 0, 0, width, height, depth, );
>> +}
>> +
>> +void
>> +util_clear_texture(struct pipe_context *pipe,
>> + struct pipe_resource *texture,
>> + const union pipe_color_union *color,
>> + unsigned level,
>> + unsigned dstx, unsigned dsty, unsigned dstz,
>> + unsigned width, unsigned height, unsigned depth)
>> +{
>> + struct pipe_transfer *dst_trans;
>> + ubyte *dst_map;
>> + enum pipe_format format = texture->format;
>>
>> + dst_map = pipe_transfer_map_3d(pipe,
>> + texture,
>> + level,
>> + PIPE_TRANSFER_WRITE,
>> + dstx, dsty, dstz,
>> + width, height, depth,
>> + _trans);
>> + if (!dst_map)
>> + return;
>> +
>> + if (dst_trans->stride > 0) {
>> + util_clear_texture_helper(dst_trans, dst_map, format, color,
>> +width, height, depth);
>> + }
>> + pipe->transfer_unmap(pipe, dst_trans);
>> +}
>>
>> #define UBYTE_TO_USHORT(B) ((B) | ((B) << 8))
>>
>> @@ -410,8 +469,6 @@ util_clear_render_target(struct pipe_context *pipe,
>> {
>> struct pipe_transfer *dst_trans;
>> ubyte *dst_map;
>> - union util_color uc;
>> - unsigned max_layer;
>>
>> assert(dst->texture);
>> if (!dst->texture)
>> @@ -426,54 +483,150 @@ util_clear_render_target(struct pipe_context *pipe,
>>unsigned pixstride = util_format_get_blocksize(dst->format);
>>dx = (dst->u.buf.first_element + dstx) * pixstride;
>>w = width * pixstride;
>> - max_layer = 0;
>>dst_map = pipe_transfer_map(pipe,
&g
[Mesa-dev] [PATCH v2] softpipe: implement clear_texture
v2: rework util clear functions such that they operate on a resource
instead of a surface (Roland Scheidegger)
Implements the ARB_clear_texture extension for softpipe.
Passes all corresponding piglit tests.
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
CC: Roland Scheidegger <srol...@vmware.com>
NOTE: someone with access will need to commit this post
review process
src/gallium/auxiliary/util/u_surface.c| 339 +-
src/gallium/auxiliary/util/u_surface.h| 17 ++
src/gallium/drivers/softpipe/sp_screen.c | 3 +-
src/gallium/drivers/softpipe/sp_texture.c | 51 +
4 files changed, 262 insertions(+), 148 deletions(-)
diff --git a/src/gallium/auxiliary/util/u_surface.c
b/src/gallium/auxiliary/util/u_surface.c
index a9ed006..1ec168f 100644
--- a/src/gallium/auxiliary/util/u_surface.c
+++ b/src/gallium/auxiliary/util/u_surface.c
@@ -388,7 +388,66 @@ no_src_map:
;
}
+static void
+util_clear_texture_helper(struct pipe_transfer *dst_trans,
+ ubyte *dst_map,
+ enum pipe_format format,
+ const union pipe_color_union *color,
+ unsigned width, unsigned height, unsigned depth)
+{
+ union util_color uc;
+
+ assert(dst_trans->stride > 0);
+
+ if (util_format_is_pure_integer(format)) {
+ /*
+ * We expect int/uint clear values here, though some APIs
+ * might disagree (but in any case util_pack_color()
+ * couldn't handle it)...
+ */
+ if (util_format_is_pure_sint(format)) {
+ util_format_write_4i(format, color->i, 0, , 0, 0, 0, 1, 1);
+ } else {
+ assert(util_format_is_pure_uint(format));
+ util_format_write_4ui(format, color->ui, 0, , 0, 0, 0, 1, 1);
+ }
+ } else {
+ util_pack_color(color->f, format, );
+ }
+
+ util_fill_box(dst_map, format,
+ dst_trans->stride, dst_trans->layer_stride,
+ 0, 0, 0, width, height, depth, );
+}
+
+void
+util_clear_texture(struct pipe_context *pipe,
+ struct pipe_resource *texture,
+ const union pipe_color_union *color,
+ unsigned level,
+ unsigned dstx, unsigned dsty, unsigned dstz,
+ unsigned width, unsigned height, unsigned depth)
+{
+ struct pipe_transfer *dst_trans;
+ ubyte *dst_map;
+ enum pipe_format format = texture->format;
+ dst_map = pipe_transfer_map_3d(pipe,
+ texture,
+ level,
+ PIPE_TRANSFER_WRITE,
+ dstx, dsty, dstz,
+ width, height, depth,
+ _trans);
+ if (!dst_map)
+ return;
+
+ if (dst_trans->stride > 0) {
+ util_clear_texture_helper(dst_trans, dst_map, format, color,
+width, height, depth);
+ }
+ pipe->transfer_unmap(pipe, dst_trans);
+}
#define UBYTE_TO_USHORT(B) ((B) | ((B) << 8))
@@ -410,8 +469,6 @@ util_clear_render_target(struct pipe_context *pipe,
{
struct pipe_transfer *dst_trans;
ubyte *dst_map;
- union util_color uc;
- unsigned max_layer;
assert(dst->texture);
if (!dst->texture)
@@ -426,54 +483,150 @@ util_clear_render_target(struct pipe_context *pipe,
unsigned pixstride = util_format_get_blocksize(dst->format);
dx = (dst->u.buf.first_element + dstx) * pixstride;
w = width * pixstride;
- max_layer = 0;
dst_map = pipe_transfer_map(pipe,
dst->texture,
0, 0,
PIPE_TRANSFER_WRITE,
dx, 0, w, 1,
_trans);
+ if (dst_map) {
+ util_clear_texture_helper(dst_trans, dst_map, dst->format, color,
+ width, height, 1);
+ pipe->transfer_unmap(pipe, dst_trans);
+ }
}
else {
- max_layer = dst->u.tex.last_layer - dst->u.tex.first_layer;
- dst_map = pipe_transfer_map_3d(pipe,
- dst->texture,
- dst->u.tex.level,
- PIPE_TRANSFER_WRITE,
- dstx, dsty, dst->u.tex.first_layer,
- width, height, max_layer + 1, _trans);
+ unsigned depth = dst->u.tex.last_layer - dst->u.tex.first_layer + 1;
+ util_clear_texture(pipe, dst->texture, color, dst->u.tex.level,
+ dstx, dsty, dst->u.tex.first_layer,
+ width, height, depth);
}
+}
+void
+util_clear_depth_stencil_texture(struct pipe_context *pipe,
+
Re: [Mesa-dev] [PATCH] softpipe: implement clear_texture
Happy to rework the implementation.
Would creating a util_clear_texture function which pulls out the
necessary components from util_clear_render_target be in alignment
with what you're imagining?
The idea would be to have util_clear_texture take a pipe_resource
instead of a pipe_surface.
Something similar would also be done for clear_depth_stencil.
Lars
On Mon, Feb 13, 2017 at 7:41 PM, Roland Scheidegger <srol...@vmware.com> wrote:
> Am 13.02.2017 um 16:20 schrieb Lars Hamre:
>> Implements the ARB_clear_texture extension for softpipe.
>> Passes all corresponding piglit tests.
>>
>> Signed-off-by: Lars Hamre <cheme...@gmail.com>
>>
>> ---
>>
>> NOTE: someone with access will need to commit this post
>> review process
>>
>> src/gallium/drivers/softpipe/sp_screen.c | 3 +-
>> src/gallium/drivers/softpipe/sp_texture.c | 60
>> +++
>> 2 files changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/drivers/softpipe/sp_screen.c
>> b/src/gallium/drivers/softpipe/sp_screen.c
>> index 02eff91..aa061d7 100644
>> --- a/src/gallium/drivers/softpipe/sp_screen.c
>> +++ b/src/gallium/drivers/softpipe/sp_screen.c
>> @@ -260,6 +260,8 @@ softpipe_get_param(struct pipe_screen *screen, enum
>> pipe_cap param)
>> case PIPE_CAP_COPY_BETWEEN_COMPRESSED_AND_PLAIN_FORMATS:
>> case PIPE_CAP_TGSI_ARRAY_COMPONENTS:
>>return 1;
>> + case PIPE_CAP_CLEAR_TEXTURE:
>> + return 1;
>> case PIPE_CAP_MULTISAMPLE_Z_RESOLVE:
>> case PIPE_CAP_RESOURCE_FROM_USER_MEMORY:
>> case PIPE_CAP_DEVICE_RESET_STATUS_QUERY:
>> @@ -268,7 +270,6 @@ softpipe_get_param(struct pipe_screen *screen, enum
>> pipe_cap param)
>> case PIPE_CAP_TGSI_TXQS:
>> case PIPE_CAP_FORCE_PERSAMPLE_INTERP:
>> case PIPE_CAP_SHAREABLE_SHADERS:
>> - case PIPE_CAP_CLEAR_TEXTURE:
>> case PIPE_CAP_DRAW_PARAMETERS:
>> case PIPE_CAP_TGSI_PACK_HALF_FLOAT:
>> case PIPE_CAP_MULTI_DRAW_INDIRECT:
>> diff --git a/src/gallium/drivers/softpipe/sp_texture.c
>> b/src/gallium/drivers/softpipe/sp_texture.c
>> index 8dca158..3794cf3 100644
>> --- a/src/gallium/drivers/softpipe/sp_texture.c
>> +++ b/src/gallium/drivers/softpipe/sp_texture.c
>> @@ -37,6 +37,7 @@
>> #include "util/u_math.h"
>> #include "util/u_memory.h"
>> #include "util/u_transfer.h"
>> +#include "util/u_surface.h"
>>
>> #include "sp_context.h"
>> #include "sp_flush.h"
>> @@ -341,6 +342,64 @@ softpipe_surface_destroy(struct pipe_context *pipe,
>> }
>>
>>
>> +static void
>> +softpipe_clear_texture(struct pipe_context *pipe,
>> + struct pipe_resource *tex,
>> + unsigned level,
>> + const struct pipe_box *box,
>> + const void *data)
>> +{
>> + struct pipe_surface tmpl = {{0}};
>> + struct pipe_surface *sf;
>> + const struct util_format_description *desc =
>> + util_format_description(tex->format);
>> +
>> + if (level > tex->last_level)
>> + return;
>> +
>> + tmpl.format = tex->format;
>> + tmpl.u.tex.first_layer = box->z;
>> + tmpl.u.tex.last_layer = box->z + box->depth - 1;
>> + tmpl.u.tex.level = level;
>> + sf = pipe->create_surface(pipe, tex, );
> I am not quite convinced of that solution. The problem is you're not
> supposed to call create_surface() on resources which didn't have the
> appropriate bind flag (although unlike llvmpipe softpipe won't warn
> about this). And in fact, there's formats where clear_texture is
> supposed to work which are definitely not renderable, so it really is an
> error to do this (even though softpipe won't actually care).
>
> But OTOH I suppose this method works...
>
> Roland
>
>
>> + if (!sf)
>> + return;
>> +
>> + if (util_format_is_depth_or_stencil(tex->format)) {
>> + unsigned clear = 0;
>> + float depth = 0.0f;
>> + uint8_t stencil = 0;
>> +
>> + if (util_format_has_depth(desc)) {
>> + clear |= PIPE_CLEAR_DEPTH;
>> + desc->unpack_z_float(, 0, data, 0, 1, 1);
>> + }
>> +
>> + if (util_format_has_stencil(desc)) {
>> + clear |= PIPE_CLEAR_STENCIL;
>> + desc->unpack_s_8uint(, 0, data, 0, 1, 1);
>> + }
>> +
>> + pipe->clear_depth_stencil(pipe, sf, clear, depth, stenci
[Mesa-dev] [PATCH] softpipe: implement clear_texture
Implements the ARB_clear_texture extension for softpipe.
Passes all corresponding piglit tests.
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
NOTE: someone with access will need to commit this post
review process
src/gallium/drivers/softpipe/sp_screen.c | 3 +-
src/gallium/drivers/softpipe/sp_texture.c | 60 +++
2 files changed, 62 insertions(+), 1 deletion(-)
diff --git a/src/gallium/drivers/softpipe/sp_screen.c
b/src/gallium/drivers/softpipe/sp_screen.c
index 02eff91..aa061d7 100644
--- a/src/gallium/drivers/softpipe/sp_screen.c
+++ b/src/gallium/drivers/softpipe/sp_screen.c
@@ -260,6 +260,8 @@ softpipe_get_param(struct pipe_screen *screen, enum
pipe_cap param)
case PIPE_CAP_COPY_BETWEEN_COMPRESSED_AND_PLAIN_FORMATS:
case PIPE_CAP_TGSI_ARRAY_COMPONENTS:
return 1;
+ case PIPE_CAP_CLEAR_TEXTURE:
+ return 1;
case PIPE_CAP_MULTISAMPLE_Z_RESOLVE:
case PIPE_CAP_RESOURCE_FROM_USER_MEMORY:
case PIPE_CAP_DEVICE_RESET_STATUS_QUERY:
@@ -268,7 +270,6 @@ softpipe_get_param(struct pipe_screen *screen, enum
pipe_cap param)
case PIPE_CAP_TGSI_TXQS:
case PIPE_CAP_FORCE_PERSAMPLE_INTERP:
case PIPE_CAP_SHAREABLE_SHADERS:
- case PIPE_CAP_CLEAR_TEXTURE:
case PIPE_CAP_DRAW_PARAMETERS:
case PIPE_CAP_TGSI_PACK_HALF_FLOAT:
case PIPE_CAP_MULTI_DRAW_INDIRECT:
diff --git a/src/gallium/drivers/softpipe/sp_texture.c
b/src/gallium/drivers/softpipe/sp_texture.c
index 8dca158..3794cf3 100644
--- a/src/gallium/drivers/softpipe/sp_texture.c
+++ b/src/gallium/drivers/softpipe/sp_texture.c
@@ -37,6 +37,7 @@
#include "util/u_math.h"
#include "util/u_memory.h"
#include "util/u_transfer.h"
+#include "util/u_surface.h"
#include "sp_context.h"
#include "sp_flush.h"
@@ -341,6 +342,64 @@ softpipe_surface_destroy(struct pipe_context *pipe,
}
+static void
+softpipe_clear_texture(struct pipe_context *pipe,
+ struct pipe_resource *tex,
+ unsigned level,
+ const struct pipe_box *box,
+ const void *data)
+{
+ struct pipe_surface tmpl = {{0}};
+ struct pipe_surface *sf;
+ const struct util_format_description *desc =
+ util_format_description(tex->format);
+
+ if (level > tex->last_level)
+ return;
+
+ tmpl.format = tex->format;
+ tmpl.u.tex.first_layer = box->z;
+ tmpl.u.tex.last_layer = box->z + box->depth - 1;
+ tmpl.u.tex.level = level;
+ sf = pipe->create_surface(pipe, tex, );
+ if (!sf)
+ return;
+
+ if (util_format_is_depth_or_stencil(tex->format)) {
+ unsigned clear = 0;
+ float depth = 0.0f;
+ uint8_t stencil = 0;
+
+ if (util_format_has_depth(desc)) {
+ clear |= PIPE_CLEAR_DEPTH;
+ desc->unpack_z_float(, 0, data, 0, 1, 1);
+ }
+
+ if (util_format_has_stencil(desc)) {
+ clear |= PIPE_CLEAR_STENCIL;
+ desc->unpack_s_8uint(, 0, data, 0, 1, 1);
+ }
+
+ pipe->clear_depth_stencil(pipe, sf, clear, depth, stencil,
+box->x, box->y,
+box->width, box->height, false);
+ } else {
+ union pipe_color_union color;
+
+ if (util_format_is_pure_uint(tex->format))
+ desc->unpack_rgba_uint(color.ui, 0, data, 0, 1, 1);
+ else if (util_format_is_pure_sint(tex->format))
+ desc->unpack_rgba_sint(color.i, 0, data, 0, 1, 1);
+ else
+ desc->unpack_rgba_float(color.f, 0, data, 0, 1, 1);
+
+ util_clear_render_target(pipe, sf, , box->x, box->y,
+ box->width, box->height);
+ }
+ pipe_surface_reference(, NULL);
+}
+
+
/**
* Geta pipe_transfer object which is used for moving data in/out of
* a resource object.
@@ -520,6 +579,7 @@ softpipe_init_texture_funcs(struct pipe_context *pipe)
pipe->create_surface = softpipe_create_surface;
pipe->surface_destroy = softpipe_surface_destroy;
+ pipe->clear_texture = softpipe_clear_texture;
}
--
2.7.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] tgsi: Enable returns from within loops
Yes please, thanks! On Tue, Sep 13, 2016 at 4:22 PM, Brian Paul <bri...@vmware.com> wrote: > On 09/13/2016 01:08 PM, Lars Hamre wrote: >> >> Fixes the following piglit test (for softpipe): >> /spec/glsl-1.10/execution/fs-loop-return >> >> Signed-off-by: Lars Hamre <cheme...@gmail.com> >> >> --- >> src/gallium/auxiliary/tgsi/tgsi_exec.c | 4 >> 1 file changed, 4 insertions(+) >> >> NOTE: Someone with access will need to commit this >>after the review process >> >> diff --git a/src/gallium/auxiliary/tgsi/tgsi_exec.c >> b/src/gallium/auxiliary/tgsi/tgsi_exec.c >> index 1457c06..aff35e6 100644 >> --- a/src/gallium/auxiliary/tgsi/tgsi_exec.c >> +++ b/src/gallium/auxiliary/tgsi/tgsi_exec.c >> @@ -5148,6 +5148,10 @@ exec_instruction( >> /* returning from main() */ >> mach->CondStackTop = 0; >> mach->LoopStackTop = 0; >> +mach->ContStackTop = 0; >> +mach->LoopLabelStackTop = 0; >> +mach->SwitchStackTop = 0; >> +mach->BreakStackTop = 0; >> *pc = -1; >> return FALSE; >>} >> -- >> 2.7.4 >> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > > Reviewed-by: Brian Paul <bri...@vmware.com> > > Do you need me to push this for you? > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] tgsi: Enable returns from within loops
Fixes the following piglit test (for softpipe): /spec/glsl-1.10/execution/fs-loop-return Signed-off-by: Lars Hamre <cheme...@gmail.com> --- src/gallium/auxiliary/tgsi/tgsi_exec.c | 4 1 file changed, 4 insertions(+) NOTE: Someone with access will need to commit this after the review process diff --git a/src/gallium/auxiliary/tgsi/tgsi_exec.c b/src/gallium/auxiliary/tgsi/tgsi_exec.c index 1457c06..aff35e6 100644 --- a/src/gallium/auxiliary/tgsi/tgsi_exec.c +++ b/src/gallium/auxiliary/tgsi/tgsi_exec.c @@ -5148,6 +5148,10 @@ exec_instruction( /* returning from main() */ mach->CondStackTop = 0; mach->LoopStackTop = 0; +mach->ContStackTop = 0; +mach->LoopLabelStackTop = 0; +mach->SwitchStackTop = 0; +mach->BreakStackTop = 0; *pc = -1; return FALSE; } -- 2.7.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] docs: Mark ARB_fragment_layer_viewport as done for softpipe
The extension is already exposed, this simply marks it as done. Signed-off-by: Lars Hamre <cheme...@gmail.com> --- Note: someone with access will need to commit this after the review process docs/GL3.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/GL3.txt b/docs/GL3.txt index ce34869..1335397 100644 --- a/docs/GL3.txt +++ b/docs/GL3.txt @@ -171,7 +171,7 @@ GL 4.3, GLSL 4.30 -- all DONE: nvc0, radeonsi GL_ARB_copy_image DONE (i965, nv50, r600, softpipe, llvmpipe) GL_KHR_debug DONE (all drivers) GL_ARB_explicit_uniform_location DONE (all drivers that support GLSL) - GL_ARB_fragment_layer_viewportDONE (i965, nv50, r600, llvmpipe) + GL_ARB_fragment_layer_viewportDONE (i965, nv50, r600, llvmpipe, softpipe) GL_ARB_framebuffer_no_attachments DONE (i965, r600, softpipe) GL_ARB_internalformat_query2 DONE (all drivers) GL_ARB_invalidate_subdata DONE (all drivers) -- 2.7.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH RESEND v4] glsl: enforce invariant conditions for built-in variables
Gentle ping. I would appreciate feedbaçk on what (if anything) I
should do to get this patch into shape.
Regards,
Lars Hamre
On Mon, Jun 6, 2016 at 9:00 AM, Lars Hamre <cheme...@gmail.com> wrote:
> v2:
> - ES version check (Tapani Pälli)
> v3/v4:
> - compare varying slot locations rather than names (Ilia Mirkin)
>
> The conditions for which certain built-in special variables
> can be declared invariant were not being checked.
>
> GLSL ES 1.00 specification, Section "Invariance and linkage" says:
>
> For the built-in special variables, gl_FragCoord can
> only be declared invariant if and only if gl_Position is
> declared invariant. Similarly gl_PointCoord can only be
> declared invariant if and only if gl_PointSize is declared
> invariant. It is an error to declare gl_FrontFacing as invariant.
>
> This fixes the following piglit tests in spec/glsl-es-1.00/linker:
> glsl-fcoord-invariant
> glsl-fface-invariant
> glsl-pcoord-invariant
>
> Signed-off-by: Lars Hamre <cheme...@gmail.com>
>
> ---
>
> NOTE: Someone with access will need to commit this after the
> review process
>
> src/compiler/glsl/link_varyings.cpp | 39
> +
> 1 file changed, 39 insertions(+)
>
> diff --git a/src/compiler/glsl/link_varyings.cpp
> b/src/compiler/glsl/link_varyings.cpp
> index 34c8906..755000e 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -336,6 +336,9 @@ cross_validate_outputs_to_inputs(struct gl_shader_program
> *prog,
> ir_variable *explicit_locations[MAX_VARYINGS_INCL_PATCH][4] =
>{ {NULL, NULL} };
>
> + bool is_gl_position_invariant = false;
> + bool is_gl_point_size_invariant = false;
> +
> /* Find all shader outputs in the "producer" stage.
> */
> foreach_in_list(ir_instruction, node, producer->ir) {
> @@ -344,6 +347,13 @@ cross_validate_outputs_to_inputs(struct
> gl_shader_program *prog,
>if (var == NULL || var->data.mode != ir_var_shader_out)
> continue;
>
> + if (prog->IsES && prog->Version < 300) {
> + if (var->data.location == VARYING_SLOT_POS)
> +is_gl_position_invariant = var->data.invariant;
> + if (var->data.location == VARYING_SLOT_PSIZ)
> +is_gl_point_size_invariant = var->data.invariant;
> + }
> +
>if (!var->data.explicit_location
>|| var->data.location < VARYING_SLOT_VAR0)
> parameters.add_variable(var);
> @@ -430,6 +440,35 @@ cross_validate_outputs_to_inputs(struct
> gl_shader_program *prog,
>if (input == NULL || input->data.mode != ir_var_shader_in)
> continue;
>
> + /*
> + * GLSL ES 1.00 specification, Section "Invariance and linkage" says:
> + *
> + * "For the built-in special variables, gl_FragCoord can
> + * only be declared invariant if and only if gl_Position is
> + * declared invariant. Similarly gl_PointCoord can only be
> + * declared invariant if and only if gl_PointSize is declared
> + * invariant. It is an error to declare gl_FrontFacing as invariant."
> + */
> + if (prog->IsES && prog->Version < 300 && input->data.invariant) {
> + if (input->data.location == VARYING_SLOT_FACE) {
> +linker_error(prog,
> + "gl_FrontFacing cannot be declared invariant");
> +return;
> + } else if (!is_gl_position_invariant &&
> +input->data.location == VARYING_SLOT_POS) {
> +linker_error(prog,
> + "gl_FragCoord cannot be declared invariant "
> + "unless gl_Position is also invariant");
> +return;
> + } else if (!is_gl_point_size_invariant &&
> +input->data.location == VARYING_SLOT_PNTC) {
> +linker_error(prog,
> + "gl_PointCoord cannot be declared invariant "
> + "unless gl_PointSize is also invariant");
> +return;
> + }
> + }
> +
>if (strcmp(input->name, "gl_Color") == 0 && input->data.used) {
> const ir_variable *const front_color =
> parameters.get_variable("gl_FrontColor");
> --
> 2.5.5
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: handle unconditional returns inside a loop
Hmmm looks like you're right. I thought I was also seeing this crash
in dri swrast, but that's not the case after a second look.
Will investigate more, thanks!
Regards,
Lars Hamre
On Tue, Jun 7, 2016 at 10:10 AM, Nicolai Hähnle <nhaeh...@gmail.com> wrote:
> On 07.06.2016 03:21, Lars Hamre wrote:
>>
>> Unrolls the loop with a count of 1 if it contains an unconditional
>> return statement.
>>
>> Fixes the following piglit test:
>> /spec/glsl-1.10/execution/fs-loop-return
>>
>> from crashing at this assert:
>> tgsi/tgsi_exec.c:5952:tgsi_exec_machine_run: Assertion `mach->ContStackTop
>> == 0' failed.
>>
>>
>> Signed-off-by: Lars Hamre <cheme...@gmail.com>
>>
>> ---
>>
>> NOTE: Someone with access will need to commit this after the
>>review process
>>
>> src/compiler/glsl/loop_unroll.cpp | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/src/compiler/glsl/loop_unroll.cpp
>> b/src/compiler/glsl/loop_unroll.cpp
>> index bc377df..b1674e4 100644
>> --- a/src/compiler/glsl/loop_unroll.cpp
>> +++ b/src/compiler/glsl/loop_unroll.cpp
>> @@ -337,6 +337,13 @@ loop_unroll_visitor::visit_leave(ir_loop *ir)
>>simple_unroll(ir, 1);
>> }
>>
>> + /* If the last instruction of the loop is a return statement,
>> + * unroll the loop with a count of 1.
>> + */
>> + if (last_ir->ir_type == ir_type_return) {
>> + simple_unroll(ir, 1);
>> + }
>
>
> I think you need to guard this against last_ir == NULL (empty loop body).
>
> Also, why is this a proper fix for an assertion in tgsi_exec? That feels
> like just hiding the real problem.
>
> Cheers,
> Nicolai
>
>> +
>> /* Don't try to unroll loops where the number of iterations is not
>> known
>> * at compile-time.
>> */
>> --
>> 2.5.5
>>
>> ___
>> 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] glsl: handle unconditional returns inside a loop
Unrolls the loop with a count of 1 if it contains an unconditional
return statement.
Fixes the following piglit test:
/spec/glsl-1.10/execution/fs-loop-return
from crashing at this assert:
tgsi/tgsi_exec.c:5952:tgsi_exec_machine_run: Assertion `mach->ContStackTop ==
0' failed.
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
NOTE: Someone with access will need to commit this after the
review process
src/compiler/glsl/loop_unroll.cpp | 7 +++
1 file changed, 7 insertions(+)
diff --git a/src/compiler/glsl/loop_unroll.cpp
b/src/compiler/glsl/loop_unroll.cpp
index bc377df..b1674e4 100644
--- a/src/compiler/glsl/loop_unroll.cpp
+++ b/src/compiler/glsl/loop_unroll.cpp
@@ -337,6 +337,13 @@ loop_unroll_visitor::visit_leave(ir_loop *ir)
simple_unroll(ir, 1);
}
+ /* If the last instruction of the loop is a return statement,
+ * unroll the loop with a count of 1.
+ */
+ if (last_ir->ir_type == ir_type_return) {
+ simple_unroll(ir, 1);
+ }
+
/* Don't try to unroll loops where the number of iterations is not known
* at compile-time.
*/
--
2.5.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH RESEND v4] glsl: enforce invariant conditions for built-in variables
v2:
- ES version check (Tapani Pälli)
v3/v4:
- compare varying slot locations rather than names (Ilia Mirkin)
The conditions for which certain built-in special variables
can be declared invariant were not being checked.
GLSL ES 1.00 specification, Section "Invariance and linkage" says:
For the built-in special variables, gl_FragCoord can
only be declared invariant if and only if gl_Position is
declared invariant. Similarly gl_PointCoord can only be
declared invariant if and only if gl_PointSize is declared
invariant. It is an error to declare gl_FrontFacing as invariant.
This fixes the following piglit tests in spec/glsl-es-1.00/linker:
glsl-fcoord-invariant
glsl-fface-invariant
glsl-pcoord-invariant
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
NOTE: Someone with access will need to commit this after the
review process
src/compiler/glsl/link_varyings.cpp | 39 +
1 file changed, 39 insertions(+)
diff --git a/src/compiler/glsl/link_varyings.cpp
b/src/compiler/glsl/link_varyings.cpp
index 34c8906..755000e 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -336,6 +336,9 @@ cross_validate_outputs_to_inputs(struct gl_shader_program
*prog,
ir_variable *explicit_locations[MAX_VARYINGS_INCL_PATCH][4] =
{ {NULL, NULL} };
+ bool is_gl_position_invariant = false;
+ bool is_gl_point_size_invariant = false;
+
/* Find all shader outputs in the "producer" stage.
*/
foreach_in_list(ir_instruction, node, producer->ir) {
@@ -344,6 +347,13 @@ cross_validate_outputs_to_inputs(struct gl_shader_program
*prog,
if (var == NULL || var->data.mode != ir_var_shader_out)
continue;
+ if (prog->IsES && prog->Version < 300) {
+ if (var->data.location == VARYING_SLOT_POS)
+is_gl_position_invariant = var->data.invariant;
+ if (var->data.location == VARYING_SLOT_PSIZ)
+is_gl_point_size_invariant = var->data.invariant;
+ }
+
if (!var->data.explicit_location
|| var->data.location < VARYING_SLOT_VAR0)
parameters.add_variable(var);
@@ -430,6 +440,35 @@ cross_validate_outputs_to_inputs(struct gl_shader_program
*prog,
if (input == NULL || input->data.mode != ir_var_shader_in)
continue;
+ /*
+ * GLSL ES 1.00 specification, Section "Invariance and linkage" says:
+ *
+ * "For the built-in special variables, gl_FragCoord can
+ * only be declared invariant if and only if gl_Position is
+ * declared invariant. Similarly gl_PointCoord can only be
+ * declared invariant if and only if gl_PointSize is declared
+ * invariant. It is an error to declare gl_FrontFacing as invariant."
+ */
+ if (prog->IsES && prog->Version < 300 && input->data.invariant) {
+ if (input->data.location == VARYING_SLOT_FACE) {
+linker_error(prog,
+ "gl_FrontFacing cannot be declared invariant");
+return;
+ } else if (!is_gl_position_invariant &&
+input->data.location == VARYING_SLOT_POS) {
+linker_error(prog,
+ "gl_FragCoord cannot be declared invariant "
+ "unless gl_Position is also invariant");
+return;
+ } else if (!is_gl_point_size_invariant &&
+input->data.location == VARYING_SLOT_PNTC) {
+linker_error(prog,
+ "gl_PointCoord cannot be declared invariant "
+ "unless gl_PointSize is also invariant");
+return;
+ }
+ }
+
if (strcmp(input->name, "gl_Color") == 0 && input->data.used) {
const ir_variable *const front_color =
parameters.get_variable("gl_FrontColor");
--
2.5.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] tgsi: use truncf in micro_trunc
Gentle ping, I would appreciate a Gallium developer's eyes on this.
Regards,
Lars Hamre
On Thu, May 26, 2016 at 6:30 PM, Lars Hamre <cheme...@gmail.com> wrote:
> Switches to using truncf in micro_trunc.
>
> Fixes the following piglit tests (for softpipe):
>
> /spec/glsl-1.30/execution/built-in-functions/...
> fs-trunc-float
> fs-trunc-vec2
> fs-trunc-vec3
> fs-trunc-vec4
> vs-trunc-float
> vs-trunc-vec2
> vs-trunc-vec3
> vs-trunc-vec4
>
> /spec/glsl-1.50/execution/built-in-functions/...
> gs-trunc-float
> gs-trunc-vec2
> gs-trunc-vec3
> gs-trunc-vec4
>
> Signed-off-by: Lars Hamre <cheme...@gmail.com>
>
> ---
>
> NOTE: someone with access will need to commit this post
> review process
>
> src/gallium/auxiliary/tgsi/tgsi_exec.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/src/gallium/auxiliary/tgsi/tgsi_exec.c
> b/src/gallium/auxiliary/tgsi/tgsi_exec.c
> index baf4a89..2ae1482 100644
> --- a/src/gallium/auxiliary/tgsi/tgsi_exec.c
> +++ b/src/gallium/auxiliary/tgsi/tgsi_exec.c
> @@ -676,10 +676,10 @@ static void
> micro_trunc(union tgsi_exec_channel *dst,
> const union tgsi_exec_channel *src)
> {
> - dst->f[0] = (float)(int)src->f[0];
> - dst->f[1] = (float)(int)src->f[1];
> - dst->f[2] = (float)(int)src->f[2];
> - dst->f[3] = (float)(int)src->f[3];
> + dst->f[0] = truncf(src->f[0]);
> + dst->f[1] = truncf(src->f[1]);
> + dst->f[2] = truncf(src->f[2]);
> + dst->f[3] = truncf(src->f[3]);
> }
>
> static void
> --
> 2.5.5
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] tgsi: use truncf in micro_trunc
Switches to using truncf in micro_trunc.
Fixes the following piglit tests (for softpipe):
/spec/glsl-1.30/execution/built-in-functions/...
fs-trunc-float
fs-trunc-vec2
fs-trunc-vec3
fs-trunc-vec4
vs-trunc-float
vs-trunc-vec2
vs-trunc-vec3
vs-trunc-vec4
/spec/glsl-1.50/execution/built-in-functions/...
gs-trunc-float
gs-trunc-vec2
gs-trunc-vec3
gs-trunc-vec4
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
NOTE: someone with access will need to commit this post
review process
src/gallium/auxiliary/tgsi/tgsi_exec.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/gallium/auxiliary/tgsi/tgsi_exec.c
b/src/gallium/auxiliary/tgsi/tgsi_exec.c
index baf4a89..2ae1482 100644
--- a/src/gallium/auxiliary/tgsi/tgsi_exec.c
+++ b/src/gallium/auxiliary/tgsi/tgsi_exec.c
@@ -676,10 +676,10 @@ static void
micro_trunc(union tgsi_exec_channel *dst,
const union tgsi_exec_channel *src)
{
- dst->f[0] = (float)(int)src->f[0];
- dst->f[1] = (float)(int)src->f[1];
- dst->f[2] = (float)(int)src->f[2];
- dst->f[3] = (float)(int)src->f[3];
+ dst->f[0] = truncf(src->f[0]);
+ dst->f[1] = truncf(src->f[1]);
+ dst->f[2] = truncf(src->f[2]);
+ dst->f[3] = truncf(src->f[3]);
}
static void
--
2.5.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] gallium/tgsi: use _mesa_roundevenf in micro_rnd
Gentle ping for a gallium developer. If nobody has any issues I would appreciate a push. Regards, Lars Hamre On Thu, May 19, 2016 at 6:16 PM, Matt Turner <matts...@gmail.com> wrote: > On Thu, May 19, 2016 at 2:34 PM, Lars Hamre <cheme...@gmail.com> wrote: >> Fixes the following piglit tests (for softpipe): >> >> /spec/glsl-1.30/execution/built-in-functions/... >> fs-roundeven-float >> fs-roundeven-vec2 >> fs-roundeven-vec3 >> fs-roundeven-vec4 >> vs-roundeven-float >> vs-roundeven-vec2 >> vs-roundeven-vec3 >> vs-roundeven-vec4 >> >> /spec/glsl-1.50/execution/built-in-functions/... >> gs-roundeven-float >> gs-roundeven-vec2 >> gs-roundeven-vec3 >> gs-roundeven-vec4 >> >> Signed-off-by: Lars Hamre <cheme...@gmail.com> >> >> --- >> >> Note: someone with access will need to commit this >> after the review process. > > I'm not going to commit it myself because I don't work on a Gallium > driver, but I'm very glad to see the patch. > > Reviewed-by: Matt Turner <matts...@gmail.com> ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v4] glsl: enforce invariant conditions for built-in variables
Ping.
On Tue, May 17, 2016 at 10:49 AM, Lars Hamre <cheme...@gmail.com> wrote:
> Gentle ping, if nobody has an issues I would appreciate a push.
>
> Regards,
> Lars Hamre
>
> On Mon, May 9, 2016 at 7:00 PM, Lars Hamre <cheme...@gmail.com> wrote:
>> v3/v4:
>> - compare varying slot locations rather than names (Ilia Mirkin)
>> v2:
>> - ES version check (Tapani Pälli)
>>
>> The conditions for which certain built-in special variables
>> can be declared invariant were not being checked.
>>
>> GLSL ES 1.00 specification, Section "Invariance and linkage" says:
>>
>> For the built-in special variables, gl_FragCoord can
>> only be declared invariant if and only if gl_Position is
>> declared invariant. Similarly gl_PointCoord can only be
>> declared invariant if and only if gl_PointSize is declared
>> invariant. It is an error to declare gl_FrontFacing as invariant.
>>
>> This fixes the following piglit tests in spec/glsl-es-1.00/linker:
>> glsl-fcoord-invariant
>> glsl-fface-invariant
>> glsl-pcoord-invariant
>>
>> Signed-off-by: Lars Hamre <cheme...@gmail.com>
>>
>> ---
>>
>> CC: Ilia Mirkin <imir...@alum.mit.edu>
>>
>> NOTE: Someone with access will need to commit this after the
>> review process
>>
>> src/compiler/glsl/link_varyings.cpp | 43
>> +++--
>> 1 file changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/compiler/glsl/link_varyings.cpp
>> b/src/compiler/glsl/link_varyings.cpp
>> index 34e82c7..c7c9f5f 100644
>> --- a/src/compiler/glsl/link_varyings.cpp
>> +++ b/src/compiler/glsl/link_varyings.cpp
>> @@ -352,13 +352,23 @@ cross_validate_outputs_to_inputs(struct
>> gl_shader_program *prog,
>> glsl_symbol_table parameters;
>> ir_variable *explicit_locations[MAX_VARYING][4] = { {NULL, NULL} };
>>
>> + bool is_gl_position_invariant = false;
>> + bool is_gl_point_size_invariant = false;
>> +
>> /* Find all shader outputs in the "producer" stage.
>> */
>> foreach_in_list(ir_instruction, node, producer->ir) {
>>ir_variable *const var = node->as_variable();
>>
>>if ((var == NULL) || (var->data.mode != ir_var_shader_out))
>> -continue;
>> + continue;
>> +
>> + if (prog->IsES && prog->Version < 300) {
>> + if (var->data.location == VARYING_SLOT_POS)
>> +is_gl_position_invariant = var->data.invariant;
>> + if (var->data.location == VARYING_SLOT_PSIZ)
>> +is_gl_point_size_invariant = var->data.invariant;
>> + }
>>
>>if (!var->data.explicit_location
>>|| var->data.location < VARYING_SLOT_VAR0)
>> @@ -442,7 +452,36 @@ cross_validate_outputs_to_inputs(struct
>> gl_shader_program *prog,
>>ir_variable *const input = node->as_variable();
>>
>>if ((input == NULL) || (input->data.mode != ir_var_shader_in))
>> -continue;
>> + continue;
>> +
>> + /*
>> + * GLSL ES 1.00 specification, Section "Invariance and linkage" says:
>> + *
>> + * "For the built-in special variables, gl_FragCoord can
>> + * only be declared invariant if and only if gl_Position is
>> + * declared invariant. Similarly gl_PointCoord can only be
>> + * declared invariant if and only if gl_PointSize is declared
>> + * invariant. It is an error to declare gl_FrontFacing as invariant."
>> + */
>> + if (prog->IsES && prog->Version < 300 && input->data.invariant) {
>> + if (input->data.location == VARYING_SLOT_FACE) {
>> +linker_error(prog,
>> + "gl_FrontFacing cannot be declared invariant");
>> +return;
>> + } else if (!is_gl_position_invariant &&
>> +input->data.location == VARYING_SLOT_POS) {
>> +linker_error(prog,
>> + "gl_FragCoord cannot be declared invariant "
>> + "unless gl_Position is also invariant");
>> +return;
>> + } else if (!is_gl_point_size_invariant &&
>> +input->data.location == VARYING_SLOT_PNTC) {
>> +linker_error(prog,
>> + "gl_PointCoord cannot be declared invariant "
>> + "unless gl_PointSize is also invariant");
>> +return;
>> + }
>> + }
>>
>>if (strcmp(input->name, "gl_Color") == 0 && input->data.used) {
>> const ir_variable *const front_color =
>> --
>> 2.5.5
>>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] gallium/tgsi: use _mesa_roundevenf in micro_rnd
Fixes the following piglit tests (for softpipe):
/spec/glsl-1.30/execution/built-in-functions/...
fs-roundeven-float
fs-roundeven-vec2
fs-roundeven-vec3
fs-roundeven-vec4
vs-roundeven-float
vs-roundeven-vec2
vs-roundeven-vec3
vs-roundeven-vec4
/spec/glsl-1.50/execution/built-in-functions/...
gs-roundeven-float
gs-roundeven-vec2
gs-roundeven-vec3
gs-roundeven-vec4
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
Note: someone with access will need to commit this
after the review process.
src/gallium/auxiliary/tgsi/tgsi_exec.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/gallium/auxiliary/tgsi/tgsi_exec.c
b/src/gallium/auxiliary/tgsi/tgsi_exec.c
index d483429..baf4a89 100644
--- a/src/gallium/auxiliary/tgsi/tgsi_exec.c
+++ b/src/gallium/auxiliary/tgsi/tgsi_exec.c
@@ -61,6 +61,7 @@
#include "util/u_half.h"
#include "util/u_memory.h"
#include "util/u_math.h"
+#include "util/rounding.h"
#define DEBUG_EXECUTION 0
@@ -543,10 +544,10 @@ static void
micro_rnd(union tgsi_exec_channel *dst,
const union tgsi_exec_channel *src)
{
- dst->f[0] = floorf(src->f[0] + 0.5f);
- dst->f[1] = floorf(src->f[1] + 0.5f);
- dst->f[2] = floorf(src->f[2] + 0.5f);
- dst->f[3] = floorf(src->f[3] + 0.5f);
+ dst->f[0] = _mesa_roundevenf(src->f[0]);
+ dst->f[1] = _mesa_roundevenf(src->f[1]);
+ dst->f[2] = _mesa_roundevenf(src->f[2]);
+ dst->f[3] = _mesa_roundevenf(src->f[3]);
}
static void
--
2.5.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v4] glsl: enforce invariant conditions for built-in variables
Gentle ping, if nobody has an issues I would appreciate a push.
Regards,
Lars Hamre
On Mon, May 9, 2016 at 7:00 PM, Lars Hamre <cheme...@gmail.com> wrote:
> v3/v4:
> - compare varying slot locations rather than names (Ilia Mirkin)
> v2:
> - ES version check (Tapani Pälli)
>
> The conditions for which certain built-in special variables
> can be declared invariant were not being checked.
>
> GLSL ES 1.00 specification, Section "Invariance and linkage" says:
>
> For the built-in special variables, gl_FragCoord can
> only be declared invariant if and only if gl_Position is
> declared invariant. Similarly gl_PointCoord can only be
> declared invariant if and only if gl_PointSize is declared
> invariant. It is an error to declare gl_FrontFacing as invariant.
>
> This fixes the following piglit tests in spec/glsl-es-1.00/linker:
> glsl-fcoord-invariant
> glsl-fface-invariant
> glsl-pcoord-invariant
>
> Signed-off-by: Lars Hamre <cheme...@gmail.com>
>
> ---
>
> CC: Ilia Mirkin <imir...@alum.mit.edu>
>
> NOTE: Someone with access will need to commit this after the
> review process
>
> src/compiler/glsl/link_varyings.cpp | 43
> +++--
> 1 file changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/src/compiler/glsl/link_varyings.cpp
> b/src/compiler/glsl/link_varyings.cpp
> index 34e82c7..c7c9f5f 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -352,13 +352,23 @@ cross_validate_outputs_to_inputs(struct
> gl_shader_program *prog,
> glsl_symbol_table parameters;
> ir_variable *explicit_locations[MAX_VARYING][4] = { {NULL, NULL} };
>
> + bool is_gl_position_invariant = false;
> + bool is_gl_point_size_invariant = false;
> +
> /* Find all shader outputs in the "producer" stage.
> */
> foreach_in_list(ir_instruction, node, producer->ir) {
>ir_variable *const var = node->as_variable();
>
>if ((var == NULL) || (var->data.mode != ir_var_shader_out))
> -continue;
> + continue;
> +
> + if (prog->IsES && prog->Version < 300) {
> + if (var->data.location == VARYING_SLOT_POS)
> +is_gl_position_invariant = var->data.invariant;
> + if (var->data.location == VARYING_SLOT_PSIZ)
> +is_gl_point_size_invariant = var->data.invariant;
> + }
>
>if (!var->data.explicit_location
>|| var->data.location < VARYING_SLOT_VAR0)
> @@ -442,7 +452,36 @@ cross_validate_outputs_to_inputs(struct
> gl_shader_program *prog,
>ir_variable *const input = node->as_variable();
>
>if ((input == NULL) || (input->data.mode != ir_var_shader_in))
> -continue;
> + continue;
> +
> + /*
> + * GLSL ES 1.00 specification, Section "Invariance and linkage" says:
> + *
> + * "For the built-in special variables, gl_FragCoord can
> + * only be declared invariant if and only if gl_Position is
> + * declared invariant. Similarly gl_PointCoord can only be
> + * declared invariant if and only if gl_PointSize is declared
> + * invariant. It is an error to declare gl_FrontFacing as invariant."
> + */
> + if (prog->IsES && prog->Version < 300 && input->data.invariant) {
> + if (input->data.location == VARYING_SLOT_FACE) {
> +linker_error(prog,
> + "gl_FrontFacing cannot be declared invariant");
> +return;
> + } else if (!is_gl_position_invariant &&
> +input->data.location == VARYING_SLOT_POS) {
> +linker_error(prog,
> + "gl_FragCoord cannot be declared invariant "
> + "unless gl_Position is also invariant");
> +return;
> + } else if (!is_gl_point_size_invariant &&
> +input->data.location == VARYING_SLOT_PNTC) {
> +linker_error(prog,
> + "gl_PointCoord cannot be declared invariant "
> + "unless gl_PointSize is also invariant");
> +return;
> + }
> + }
>
>if (strcmp(input->name, "gl_Color") == 0 && input->data.used) {
> const ir_variable *const front_color =
> --
> 2.5.5
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v4] glsl: enforce invariant conditions for built-in variables
v3/v4:
- compare varying slot locations rather than names (Ilia Mirkin)
v2:
- ES version check (Tapani Pälli)
The conditions for which certain built-in special variables
can be declared invariant were not being checked.
GLSL ES 1.00 specification, Section "Invariance and linkage" says:
For the built-in special variables, gl_FragCoord can
only be declared invariant if and only if gl_Position is
declared invariant. Similarly gl_PointCoord can only be
declared invariant if and only if gl_PointSize is declared
invariant. It is an error to declare gl_FrontFacing as invariant.
This fixes the following piglit tests in spec/glsl-es-1.00/linker:
glsl-fcoord-invariant
glsl-fface-invariant
glsl-pcoord-invariant
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
CC: Ilia Mirkin <imir...@alum.mit.edu>
NOTE: Someone with access will need to commit this after the
review process
src/compiler/glsl/link_varyings.cpp | 43 +++--
1 file changed, 41 insertions(+), 2 deletions(-)
diff --git a/src/compiler/glsl/link_varyings.cpp
b/src/compiler/glsl/link_varyings.cpp
index 34e82c7..c7c9f5f 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -352,13 +352,23 @@ cross_validate_outputs_to_inputs(struct gl_shader_program
*prog,
glsl_symbol_table parameters;
ir_variable *explicit_locations[MAX_VARYING][4] = { {NULL, NULL} };
+ bool is_gl_position_invariant = false;
+ bool is_gl_point_size_invariant = false;
+
/* Find all shader outputs in the "producer" stage.
*/
foreach_in_list(ir_instruction, node, producer->ir) {
ir_variable *const var = node->as_variable();
if ((var == NULL) || (var->data.mode != ir_var_shader_out))
-continue;
+ continue;
+
+ if (prog->IsES && prog->Version < 300) {
+ if (var->data.location == VARYING_SLOT_POS)
+is_gl_position_invariant = var->data.invariant;
+ if (var->data.location == VARYING_SLOT_PSIZ)
+is_gl_point_size_invariant = var->data.invariant;
+ }
if (!var->data.explicit_location
|| var->data.location < VARYING_SLOT_VAR0)
@@ -442,7 +452,36 @@ cross_validate_outputs_to_inputs(struct gl_shader_program
*prog,
ir_variable *const input = node->as_variable();
if ((input == NULL) || (input->data.mode != ir_var_shader_in))
-continue;
+ continue;
+
+ /*
+ * GLSL ES 1.00 specification, Section "Invariance and linkage" says:
+ *
+ * "For the built-in special variables, gl_FragCoord can
+ * only be declared invariant if and only if gl_Position is
+ * declared invariant. Similarly gl_PointCoord can only be
+ * declared invariant if and only if gl_PointSize is declared
+ * invariant. It is an error to declare gl_FrontFacing as invariant."
+ */
+ if (prog->IsES && prog->Version < 300 && input->data.invariant) {
+ if (input->data.location == VARYING_SLOT_FACE) {
+linker_error(prog,
+ "gl_FrontFacing cannot be declared invariant");
+return;
+ } else if (!is_gl_position_invariant &&
+input->data.location == VARYING_SLOT_POS) {
+linker_error(prog,
+ "gl_FragCoord cannot be declared invariant "
+ "unless gl_Position is also invariant");
+return;
+ } else if (!is_gl_point_size_invariant &&
+input->data.location == VARYING_SLOT_PNTC) {
+linker_error(prog,
+ "gl_PointCoord cannot be declared invariant "
+ "unless gl_PointSize is also invariant");
+return;
+ }
+ }
if (strcmp(input->name, "gl_Color") == 0 && input->data.used) {
const ir_variable *const front_color =
--
2.5.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3] glsl: enforce invariant conditions for built-in variables
Looks like it, I didn't realized VARYING_SLOT_POS covered both cases.
On Mon, May 9, 2016 at 5:17 PM, Ilia Mirkin <imir...@alum.mit.edu> wrote:
> On Mon, May 9, 2016 at 5:10 PM, Lars Hamre <cheme...@gmail.com> wrote:
>> v3:
>> - compare varying slot locations rather than names (Ilia Mirkin)
>> v2:
>> - ES version check (Tapani Pälli)
>>
>> The conditions for which certain built-in special variables
>> can be declared invariant were not being checked.
>>
>> GLSL ES 1.00 specification, Section "Invariance and linkage" says:
>>
>> For the built-in special variables, gl_FragCoord can
>> only be declared invariant if and only if gl_Position is
>> declared invariant. Similarly gl_PointCoord can only be
>> declared invariant if and only if gl_PointSize is declared
>> invariant. It is an error to declare gl_FrontFacing as invariant.
>>
>> This fixes the following piglit tests in spec/glsl-es-1.00/linker:
>> glsl-fcoord-invariant
>> glsl-fface-invariant
>> glsl-pcoord-invariant
>>
>> Signed-off-by: Lars Hamre <cheme...@gmail.com>
>>
>> ---
>>
>> CC: Ilia Mirkin <imir...@alum.mit.edu>
>>
>> NOTE: Someone with access will need to commit this after the
>> review process
>>
>> src/compiler/glsl/link_varyings.cpp | 43
>> +++--
>> 1 file changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/compiler/glsl/link_varyings.cpp
>> b/src/compiler/glsl/link_varyings.cpp
>> index 34e82c7..155a4d9 100644
>> --- a/src/compiler/glsl/link_varyings.cpp
>> +++ b/src/compiler/glsl/link_varyings.cpp
>> @@ -352,13 +352,23 @@ cross_validate_outputs_to_inputs(struct
>> gl_shader_program *prog,
>> glsl_symbol_table parameters;
>> ir_variable *explicit_locations[MAX_VARYING][4] = { {NULL, NULL} };
>>
>> + bool is_gl_position_invariant = false;
>> + bool is_gl_point_size_invariant = false;
>> +
>> /* Find all shader outputs in the "producer" stage.
>> */
>> foreach_in_list(ir_instruction, node, producer->ir) {
>>ir_variable *const var = node->as_variable();
>>
>>if ((var == NULL) || (var->data.mode != ir_var_shader_out))
>> -continue;
>> + continue;
>> +
>> + if (prog->IsES && prog->Version < 300) {
>> + if (var->data.location == VARYING_SLOT_POS)
>> +is_gl_position_invariant = var->data.invariant;
>> + if (var->data.location == VARYING_SLOT_PSIZ)
>> +is_gl_point_size_invariant = var->data.invariant;
>> + }
>>
>>if (!var->data.explicit_location
>>|| var->data.location < VARYING_SLOT_VAR0)
>> @@ -442,7 +452,36 @@ cross_validate_outputs_to_inputs(struct
>> gl_shader_program *prog,
>>ir_variable *const input = node->as_variable();
>>
>>if ((input == NULL) || (input->data.mode != ir_var_shader_in))
>> -continue;
>> + continue;
>> +
>> + /*
>> + * GLSL ES 1.00 specification, Section "Invariance and linkage" says:
>> + *
>> + * "For the built-in special variables, gl_FragCoord can
>> + * only be declared invariant if and only if gl_Position is
>> + * declared invariant. Similarly gl_PointCoord can only be
>> + * declared invariant if and only if gl_PointSize is declared
>> + * invariant. It is an error to declare gl_FrontFacing as invariant."
>> + */
>> + if (prog->IsES && prog->Version < 300 && input->data.invariant) {
>> + if (input->data.location == VARYING_SLOT_FACE) {
>> +linker_error(prog,
>> + "gl_FrontFacing cannot be declared invariant");
>> +return;
>> + } else if (!is_gl_position_invariant &&
>> +!strcmp(input->name, "gl_FragCoord")) {
>
> I think you want input->data.location == VARYING_SLOT_POS here.
>
>> +linker_error(prog,
>> + "gl_FragCoord cannot be declared invariant "
>> + "unless gl_Position is also invariant");
>> +return;
>> + } else if (!is_gl_point_size_invariant &&
>> +input->data.location == VARYING_SLOT_PNTC) {
>> +linker_error(prog,
>> + "gl_PointCoord cannot be declared invariant "
>> + "unless gl_PointSize is also invariant");
>> +return;
>> + }
>> + }
>>
>>if (strcmp(input->name, "gl_Color") == 0 && input->data.used) {
>> const ir_variable *const front_color =
>> --
>> 2.5.5
>>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v3] glsl: enforce invariant conditions for built-in variables
v3:
- compare varying slot locations rather than names (Ilia Mirkin)
v2:
- ES version check (Tapani Pälli)
The conditions for which certain built-in special variables
can be declared invariant were not being checked.
GLSL ES 1.00 specification, Section "Invariance and linkage" says:
For the built-in special variables, gl_FragCoord can
only be declared invariant if and only if gl_Position is
declared invariant. Similarly gl_PointCoord can only be
declared invariant if and only if gl_PointSize is declared
invariant. It is an error to declare gl_FrontFacing as invariant.
This fixes the following piglit tests in spec/glsl-es-1.00/linker:
glsl-fcoord-invariant
glsl-fface-invariant
glsl-pcoord-invariant
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
CC: Ilia Mirkin <imir...@alum.mit.edu>
NOTE: Someone with access will need to commit this after the
review process
src/compiler/glsl/link_varyings.cpp | 43 +++--
1 file changed, 41 insertions(+), 2 deletions(-)
diff --git a/src/compiler/glsl/link_varyings.cpp
b/src/compiler/glsl/link_varyings.cpp
index 34e82c7..155a4d9 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -352,13 +352,23 @@ cross_validate_outputs_to_inputs(struct gl_shader_program
*prog,
glsl_symbol_table parameters;
ir_variable *explicit_locations[MAX_VARYING][4] = { {NULL, NULL} };
+ bool is_gl_position_invariant = false;
+ bool is_gl_point_size_invariant = false;
+
/* Find all shader outputs in the "producer" stage.
*/
foreach_in_list(ir_instruction, node, producer->ir) {
ir_variable *const var = node->as_variable();
if ((var == NULL) || (var->data.mode != ir_var_shader_out))
-continue;
+ continue;
+
+ if (prog->IsES && prog->Version < 300) {
+ if (var->data.location == VARYING_SLOT_POS)
+is_gl_position_invariant = var->data.invariant;
+ if (var->data.location == VARYING_SLOT_PSIZ)
+is_gl_point_size_invariant = var->data.invariant;
+ }
if (!var->data.explicit_location
|| var->data.location < VARYING_SLOT_VAR0)
@@ -442,7 +452,36 @@ cross_validate_outputs_to_inputs(struct gl_shader_program
*prog,
ir_variable *const input = node->as_variable();
if ((input == NULL) || (input->data.mode != ir_var_shader_in))
-continue;
+ continue;
+
+ /*
+ * GLSL ES 1.00 specification, Section "Invariance and linkage" says:
+ *
+ * "For the built-in special variables, gl_FragCoord can
+ * only be declared invariant if and only if gl_Position is
+ * declared invariant. Similarly gl_PointCoord can only be
+ * declared invariant if and only if gl_PointSize is declared
+ * invariant. It is an error to declare gl_FrontFacing as invariant."
+ */
+ if (prog->IsES && prog->Version < 300 && input->data.invariant) {
+ if (input->data.location == VARYING_SLOT_FACE) {
+linker_error(prog,
+ "gl_FrontFacing cannot be declared invariant");
+return;
+ } else if (!is_gl_position_invariant &&
+!strcmp(input->name, "gl_FragCoord")) {
+linker_error(prog,
+ "gl_FragCoord cannot be declared invariant "
+ "unless gl_Position is also invariant");
+return;
+ } else if (!is_gl_point_size_invariant &&
+input->data.location == VARYING_SLOT_PNTC) {
+linker_error(prog,
+ "gl_PointCoord cannot be declared invariant "
+ "unless gl_PointSize is also invariant");
+return;
+ }
+ }
if (strcmp(input->name, "gl_Color") == 0 && input->data.used) {
const ir_variable *const front_color =
--
2.5.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2] glsl: enforce invariant conditions for built-in variables
v2:
- ES version check (Tapani Pälli)
The conditions for which certain built-in special variables
can be declared invariant were not being checked.
GLSL ES 1.00 specification, Section "Invariance and linkage" says:
For the built-in special variables, gl_FragCoord can
only be declared invariant if and only if gl_Position is
declared invariant. Similarly gl_PointCoord can only be
declared invariant if and only if gl_PointSize is declared
invariant. It is an error to declare gl_FrontFacing as invariant.
This fixes the following piglit tests in spec/glsl-es-1.00/linker:
glsl-fcoord-invariant
glsl-fface-invariant
glsl-pcoord-invariant
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
CC: Tapani Pälli <tapani.pa...@intel.com>
NOTE: Someone with access will need to commit this after the
review process
src/compiler/glsl/link_varyings.cpp | 46 +++--
1 file changed, 44 insertions(+), 2 deletions(-)
diff --git a/src/compiler/glsl/link_varyings.cpp
b/src/compiler/glsl/link_varyings.cpp
index 34e82c7..2c1f57d 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -352,13 +352,23 @@ cross_validate_outputs_to_inputs(struct gl_shader_program
*prog,
glsl_symbol_table parameters;
ir_variable *explicit_locations[MAX_VARYING][4] = { {NULL, NULL} };
+ bool is_gl_position_invariant = false;
+ bool is_gl_point_size_invariant = false;
+
/* Find all shader outputs in the "producer" stage.
*/
foreach_in_list(ir_instruction, node, producer->ir) {
ir_variable *const var = node->as_variable();
if ((var == NULL) || (var->data.mode != ir_var_shader_out))
-continue;
+ continue;
+
+ if (prog->IsES && prog->Version < 300) {
+ if (!strcmp(var->name, "gl_Position"))
+is_gl_position_invariant = var->data.invariant;
+ if (!strcmp(var->name, "gl_PointSize"))
+is_gl_point_size_invariant = var->data.invariant;
+ }
if (!var->data.explicit_location
|| var->data.location < VARYING_SLOT_VAR0)
@@ -442,7 +452,39 @@ cross_validate_outputs_to_inputs(struct gl_shader_program
*prog,
ir_variable *const input = node->as_variable();
if ((input == NULL) || (input->data.mode != ir_var_shader_in))
-continue;
+ continue;
+
+ /*
+ * GLSL ES 1.00 specification, Section "Invariance and linkage" says:
+ *
+ * "For the built-in special variables, gl_FragCoord can
+ * only be declared invariant if and only if gl_Position is
+ * declared invariant. Similarly gl_PointCoord can only be
+ * declared invariant if and only if gl_PointSize is declared
+ * invariant. It is an error to declare gl_FrontFacing as invariant."
+ */
+ if (prog->IsES && prog->Version < 300) {
+ if (!strcmp(input->name, "gl_FrontFacing") &&
+ input->data.invariant) {
+linker_error(prog,
+ "gl_FrontFacing cannot be declared invariant");
+return;
+ } else if (!strcmp(input->name, "gl_FragCoord") &&
+input->data.invariant &&
+!is_gl_position_invariant) {
+linker_error(prog,
+ "gl_FragCoord cannot be declared invariant "
+ "unless gl_Position is also invariant");
+return;
+ } else if (!strcmp(input->name, "gl_PointCoord") &&
+input->data.invariant &&
+!is_gl_point_size_invariant) {
+linker_error(prog,
+ "gl_PointCoord cannot be declared invariant "
+ "unless gl_PointSize is also invariant");
+return;
+ }
+ }
if (strcmp(input->name, "gl_Color") == 0 && input->data.used) {
const ir_variable *const front_color =
--
2.5.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] glsl: Copy propagate in array index function parameters
Reviewed-by: Lars Hamre <cheme...@gmail.com>
On Fri, May 6, 2016 at 11:54 AM, Juan A. Suarez Romero
<jasua...@igalia.com> wrote:
> Copy propagate is not applied in function parameters when they are out
> or inout.
>
> But if the parameter is an array, we can copy propagate the index array.
>
> This also fixes shaders@out-parameter-indexing piglit tests, that
> exposes a wrong handling of inout function parameters in Mesa.
>
> This commit does not produce any piglit regression, nor any change in
> shader-db results.
>
> v2:
> - Deal with swizzles (jasuarez)
> - Use a loop top peel off all array dereferences (Ilia)
> ---
> src/compiler/glsl/opt_copy_propagation.cpp | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/src/compiler/glsl/opt_copy_propagation.cpp
> b/src/compiler/glsl/opt_copy_propagation.cpp
> index ae62921..e464de3 100644
> --- a/src/compiler/glsl/opt_copy_propagation.cpp
> +++ b/src/compiler/glsl/opt_copy_propagation.cpp
> @@ -193,6 +193,24 @@ ir_copy_propagation_visitor::visit_enter(ir_call *ir)
>if (sig_param->data.mode != ir_var_function_out
>&& sig_param->data.mode != ir_var_function_inout) {
> ir->accept(this);
> + } else {
> + /* Peeling of all array dereferences / swizzles */
> + ir_dereference_array *ir_array;
> + ir_swizzle *ir_swizzle;
> + do {
> +ir_array = ir->as_dereference_array();
> +if (ir_array) {
> + ir_array->array_index->accept(this);
> + ir = ir_array->array;
> + continue;
> +}
> +ir_swizzle = ir->as_swizzle();
> +if (ir_swizzle) {
> + ir = ir_swizzle->val;
> + continue;
> +}
> +ir = NULL;
> + } while (ir);
>}
> }
>
> --
> 2.5.5
>
> ___
> 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] glsl: enforce invariant conditions for built-in variables
The conditions for which certain built-in special variables
can be declared invariant were not being checked.
OpenGL ES 1.00 specification "Invariance and linkage":
For the built-in special variables, gl_FragCoord can
only be declared invariant if and only if gl_Position is
declared invariant. Similarly gl_PointCoord can only be
declared invariant if and only if gl_PointSize is declared
invariant. It is an error to declare gl_FrontFacing as invariant.
This fixes the following piglit tests in spec/glsl-es-1.00/linker:
glsl-fcoord-invariant
glsl-fface-invariant
glsl-pcoord-invariant
---
NOTE: Someone with access will need to commit this after the
review process
src/compiler/glsl/link_varyings.cpp | 46 +++--
1 file changed, 44 insertions(+), 2 deletions(-)
diff --git a/src/compiler/glsl/link_varyings.cpp
b/src/compiler/glsl/link_varyings.cpp
index 34e82c7..8ef815c 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -352,13 +352,23 @@ cross_validate_outputs_to_inputs(struct gl_shader_program
*prog,
glsl_symbol_table parameters;
ir_variable *explicit_locations[MAX_VARYING][4] = { {NULL, NULL} };
+ bool is_gl_position_invariant = false;
+ bool is_gl_point_size_invariant = false;
+
/* Find all shader outputs in the "producer" stage.
*/
foreach_in_list(ir_instruction, node, producer->ir) {
ir_variable *const var = node->as_variable();
if ((var == NULL) || (var->data.mode != ir_var_shader_out))
-continue;
+ continue;
+
+ if (prog->IsES) {
+ if (!strcmp(var->name, "gl_Position"))
+is_gl_position_invariant = var->data.invariant;
+ if (!strcmp(var->name, "gl_PointSize"))
+is_gl_point_size_invariant = var->data.invariant;
+ }
if (!var->data.explicit_location
|| var->data.location < VARYING_SLOT_VAR0)
@@ -442,7 +452,39 @@ cross_validate_outputs_to_inputs(struct gl_shader_program
*prog,
ir_variable *const input = node->as_variable();
if ((input == NULL) || (input->data.mode != ir_var_shader_in))
-continue;
+ continue;
+
+ /*
+ * OpenGL ES 1.00 specification "Invariance and linkage":
+ *
+ * "For the built-in special variables, gl_FragCoord can
+ * only be declared invariant if and only if gl_Position is
+ * declared invariant. Similarly gl_PointCoord can only be
+ * declared invariant if and only if gl_PointSize is declared
+ * invariant. It is an error to declare gl_FrontFacing as invariant."
+ */
+ if (prog->IsES) {
+ if (!strcmp(input->name, "gl_FrontFacing") &&
+ input->data.invariant) {
+linker_error(prog,
+ "gl_FrontFacing cannot be declared invariant");
+return;
+ } else if (!strcmp(input->name, "gl_FragCoord") &&
+input->data.invariant &&
+!is_gl_position_invariant) {
+linker_error(prog,
+ "gl_FragCoord cannot be declared invariant "
+ "unless gl_Position is also invariant");
+return;
+ } else if (!strcmp(input->name, "gl_PointCoord") &&
+input->data.invariant &&
+!is_gl_point_size_invariant) {
+linker_error(prog,
+ "gl_PointCoord cannot be declared invariant "
+ "unless gl_PointSize is also invariant");
+return;
+ }
+ }
if (strcmp(input->name, "gl_Color") == 0 && input->data.used) {
const ir_variable *const front_color =
--
2.5.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: correctly handle inout parameters post function inlining
Sounds good to me, go ahead and push the new version! On Fri, May 6, 2016 at 10:44 AM, Juan A. Suarez Romero <jasua...@igalia.com> wrote: > On Fri, 2016-05-06 at 10:39 -0400, Lars Hamre wrote: >> Hi Juan, >> >> Sorry I missed that. >> >> It looks like your patch doesn't fix the out parameter indexing for: >> vs-inout-index-inout-mat2-col >> vs-inout-index-inout-vec4-array >> >> I was able to extend your patch to get these tests passing by: >> - if the ir_array->array was a dereferenced_array, copy propagate >> it's index >> - if the ir->ir_type == ir_type_swizzle, if it's val was a >> dereferenced_array, >> copying propagate it's index > > That's great! Precisely, I have here a version that fixes those two > test by doing exactly that :D > > > I can push a new version with that change, or if you prefer doing it > yourself, just let me know > > > J.A. > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: correctly handle inout parameters post function inlining
Hi Juan, Sorry I missed that. It looks like your patch doesn't fix the out parameter indexing for: vs-inout-index-inout-mat2-col vs-inout-index-inout-vec4-array I was able to extend your patch to get these tests passing by: - if the ir_array->array was a dereferenced_array, copy propagate it's index - if the ir->ir_type == ir_type_swizzle, if it's val was a dereferenced_array, copying propagate it's index I would rather move ahead with your patch as mine seems quite "hacky" in comparison :) Regards, Lars Hamre On Fri, May 6, 2016 at 9:53 AM, Juan A. Suarez Romero <jasua...@igalia.com> wrote: > On Fri, 2016-05-06 at 08:49 -0400, Lars Hamre wrote: >> Inout parameters which depended on other inout parameters >> where not assigned in the correct order. >> >> Fixes the following piglit tests in shaders/out-parameter-indexing: >> vs-inout-index-inout-float-array >> vs-inout-index-inout-mat2-col >> vs-inout-index-inout-mat2-row >> vs-inout-index-inout-vec4 >> vs-inout-index-inout-vec4-array >> vs-inout-index-inout-vec4-array-element > > > Some days ago I sent a patch that also fixes some of these tests in a > different way. > > https://lists.freedesktop.org/archives/mesa-dev/2016-May/115621.html > > > > J.A. > > ___ > 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] glsl: correctly handle inout parameters post function inlining
Inout parameters which depended on other inout parameters
where not assigned in the correct order.
Fixes the following piglit tests in shaders/out-parameter-indexing:
vs-inout-index-inout-float-array
vs-inout-index-inout-mat2-col
vs-inout-index-inout-mat2-row
vs-inout-index-inout-vec4
vs-inout-index-inout-vec4-array
vs-inout-index-inout-vec4-array-element
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
NOTES:
- Someone with access will need to commit this post
review process.
- Not sure if this is the "mesa" approach to this
problem, feedback is appreciated.
src/compiler/glsl/opt_function_inlining.cpp | 80 ++---
1 file changed, 73 insertions(+), 7 deletions(-)
diff --git a/src/compiler/glsl/opt_function_inlining.cpp
b/src/compiler/glsl/opt_function_inlining.cpp
index 19f5fae..a5e7ea6 100644
--- a/src/compiler/glsl/opt_function_inlining.cpp
+++ b/src/compiler/glsl/opt_function_inlining.cpp
@@ -95,6 +95,54 @@ replace_return_with_assignment(ir_instruction *ir, void
*data)
}
}
+static void
+gather_inout_vars_from_params(void *ctx,
+ exec_list *actual_params,
+ exec_list *callee_params,
+ exec_list *inout_vars)
+{
+ foreach_two_lists(formal_node, callee_params,
+ actual_node, actual_params) {
+ ir_rvalue *const param = (ir_rvalue *)actual_node;
+ const ir_variable *const sig_param = (ir_variable *)formal_node;
+
+ if (sig_param->data.mode == ir_var_function_inout &&
+param->ir_type == ir_type_dereference_variable) {
+ inout_vars->push_tail(sig_param->clone(ctx, NULL));
+ }
+ }
+}
+
+static bool
+rvalue_depends_on_inout_var_helper(ir_rvalue *rv, exec_list *inout_vars)
+{
+ if (rv->ir_type == ir_type_dereference_array) {
+ ir_dereference_array *deref_arr = (ir_dereference_array *)rv;
+ return (rvalue_depends_on_inout_var_helper(deref_arr->array, inout_vars)
||
+ rvalue_depends_on_inout_var_helper(deref_arr->array_index,
inout_vars));
+ } else if (rv->ir_type == ir_type_swizzle) {
+ return rvalue_depends_on_inout_var_helper(((ir_swizzle *)rv)->val,
inout_vars);
+ } else if (rv->ir_type == ir_type_dereference_variable) {
+ const char *var_name = ((ir_dereference_variable *)rv)->var->name;
+ foreach_in_list(ir_variable, inout_var, inout_vars) {
+ if (!strcmp(var_name, inout_var->name)) {
+return true;
+ }
+ }
+ }
+ return false;
+}
+
+static bool
+rvalue_depends_on_inout_var(ir_rvalue *rv, exec_list *inout_vars)
+{
+ if (rv->ir_type == ir_type_dereference_array ||
+ rv->ir_type == ir_type_swizzle) {
+ return rvalue_depends_on_inout_var_helper(rv, inout_vars);
+ }
+ return false;
+}
+
void
ir_call::generate_inline(ir_instruction *next_ir)
{
@@ -185,7 +233,14 @@ ir_call::generate_inline(ir_instruction *next_ir)
/* Copy back the value of any 'out' parameters from the function body
* variables to our own.
*/
+
+ exec_list inout_vars;
+ gather_inout_vars_from_params(ctx, >actual_parameters,
+ >callee->parameters,
+ _vars);
+
i = 0;
+ ir_instruction *last_non_dependent_inout_assigned = next_ir;
foreach_two_lists(formal_node, >callee->parameters,
actual_node, >actual_parameters) {
ir_rvalue *const param = (ir_rvalue *) actual_node;
@@ -194,12 +249,24 @@ ir_call::generate_inline(ir_instruction *next_ir)
/* Move our param variable into the actual param if it's an 'out' type.
*/
if (parameters[i] && (sig_param->data.mode == ir_var_function_out ||
sig_param->data.mode == ir_var_function_inout)) {
-ir_assignment *assign;
-
-assign = new(ctx) ir_assignment(param->clone(ctx, NULL)->as_rvalue(),
-new(ctx)
ir_dereference_variable(parameters[i]),
-NULL);
-next_ir->insert_before(assign);
+ ir_assignment *assign;
+
+ assign = new(ctx) ir_assignment(param->clone(ctx, NULL)->as_rvalue(),
+ new(ctx)
ir_dereference_variable(parameters[i]),
+ NULL);
+
+ if (sig_param->data.mode == ir_var_function_inout &&
+ rvalue_depends_on_inout_var(param, _vars)) {
+/*
+ * Insert assignment before the others if it depends on another
+ * inout parameter.
+ */
+last_non_dependent_inout_assigned->insert_before(assign);
+ } else {
+/* Otherwise insert assignment before the next_ir */
+next_ir->insert_before(assign);
+last_non_depe
Re: [Mesa-dev] [PATCH v2] glsl: fix lowering outputs for early/nested returns
Thanks again, I will make/modify those piglit tests.
Regards,
Lars Hamre
On Tue, Apr 26, 2016 at 9:20 PM, Timothy Arceri
<timothy.arc...@collabora.com> wrote:
> On Tue, 2016-04-26 at 19:50 -0400, Lars Hamre wrote:
>> v2: limit lowerings to return statments in main()
>>
>> Return statements in conditional blocks were not having their
>> output varyings lowered correctly.
>>
>> This patch fixes the following piglit tests:
>> /spec/glsl-1.10/execution/vs-float-main-return
>> /spec/glsl-1.10/execution/vs-vec2-main-return
>> /spec/glsl-1.10/execution/vs-vec3-main-return
>>
>> Signed-off-by: Lars Hamre <cheme...@gmail.com>
>>
>> ---
>>
>> CC: Timothy Arceri <timothy.arc...@collabora.com>
>>
>> Hi Timothy,
>>
>> As it turns out, functions are inlined prior to
>> lower_packed_varyings() being called.
>
> Ok thanks for checking.
>
>> I put in the check
>> to not visit other functions anyways in case that changes
>> at some point in the future.
>
> We can probably just go with V1. I'm not really a fan of extra code
> that doesn't get used.
>
>>
>> I could not determine a way to test having the splicer
>> inserting instructions into a function that is not main()
>> within piglit. Inserting the lowering instructions before
>> a function's return statement just inserts "useless"
>> instructions, but does not produce incorrect results.
>> Please nudge me in the right direction if you have an idea.
>
> I had convinced myself yesterday it could be tested but your right the
> values should just be overwritten again before we can ever access them.
> Guess I should wait til I've woken up a bit more in the morning before
> reviewing patches :P
>
>>
>> There already exists piglit tests which check float, vec2,
>> and vec3 varyings for early returns (which this patch
>> fixes). Let me know if you have anything else to test for
>> in mind.
>
> The test I suggested in my review is slightly different from the
> existing tests:
>
> foo1 = vec2(0.5);
> if (early_return != 0) {
> foo1 = vec2(0.2);
> return;
> }
>
> vs
>
> foo1 = vec2(0.5);
> if (early_return != 0)
> return;
> foo1 = vec2(0.2);
>
> I think your code should handle it ok but it would be nice to test this
> alternative also. My concern was that the last instruction is now the
> return so the copy instructions might not get added, although I think
> it should be ok as your code should only see the if.
>
> The other thing that would be nice is to update the existing tests to
> also check the alternative path. So after the existing probe you would
> add 'uniform int early_return 0' and test for 0.2.
>
> As for this patch I'll add my R-B to the first version and push it in a
> little bit if there is no further feedback.
>
>>
>> Regards,
>> Lars Hamre
>>
>> NOTE: Someone with access will need to commit this after the review
>> process
>>
>> src/compiler/glsl/lower_packed_varyings.cpp | 68
>> +++--
>> 1 file changed, 64 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/compiler/glsl/lower_packed_varyings.cpp
>> b/src/compiler/glsl/lower_packed_varyings.cpp
>> index 825cc9e..c9197b7 100644
>> --- a/src/compiler/glsl/lower_packed_varyings.cpp
>> +++ b/src/compiler/glsl/lower_packed_varyings.cpp
>> @@ -724,6 +724,55 @@
>> lower_packed_varyings_gs_splicer::visit_leave(ir_emit_vertex *ev)
>> return visit_continue;
>> }
>>
>> +/**
>> + * Visitor that splices varying packing code before every return in
>> main().
>> + */
>> +class lower_packed_varyings_return_splicer : public
>> ir_hierarchical_visitor
>> +{
>> +public:
>> + explicit lower_packed_varyings_return_splicer(void *mem_ctx,
>> + const exec_list
>> *instructions);
>> +
>> + virtual ir_visitor_status visit_leave(ir_return *ret);
>> + virtual ir_visitor_status visit_enter(ir_function_signature
>> *sig);
>> +
>> +private:
>> + /**
>> +* Memory context used to allocate new instructions for the
>> shader.
>> +*/
>> + void * const mem_ctx;
>> +
>> + /**
>> +* Instructions that should be spliced into place before each
>> return.
>> +*/
>> + const exec_list *instructions;
>> +};
>> +
>> +
>> +lower_packed_varyings_return_splicer::lower_packed_va
[Mesa-dev] [PATCH v2] glsl: fix lowering outputs for early/nested returns
v2: limit lowerings to return statments in main()
Return statements in conditional blocks were not having their
output varyings lowered correctly.
This patch fixes the following piglit tests:
/spec/glsl-1.10/execution/vs-float-main-return
/spec/glsl-1.10/execution/vs-vec2-main-return
/spec/glsl-1.10/execution/vs-vec3-main-return
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
CC: Timothy Arceri <timothy.arc...@collabora.com>
Hi Timothy,
As it turns out, functions are inlined prior to
lower_packed_varyings() being called. I put in the check
to not visit other functions anyways in case that changes
at some point in the future.
I could not determine a way to test having the splicer
inserting instructions into a function that is not main()
within piglit. Inserting the lowering instructions before
a function's return statement just inserts "useless"
instructions, but does not produce incorrect results.
Please nudge me in the right direction if you have an idea.
There already exists piglit tests which check float, vec2,
and vec3 varyings for early returns (which this patch
fixes). Let me know if you have anything else to test for
in mind.
Regards,
Lars Hamre
NOTE: Someone with access will need to commit this after the review
process
src/compiler/glsl/lower_packed_varyings.cpp | 68 +++--
1 file changed, 64 insertions(+), 4 deletions(-)
diff --git a/src/compiler/glsl/lower_packed_varyings.cpp
b/src/compiler/glsl/lower_packed_varyings.cpp
index 825cc9e..c9197b7 100644
--- a/src/compiler/glsl/lower_packed_varyings.cpp
+++ b/src/compiler/glsl/lower_packed_varyings.cpp
@@ -724,6 +724,55 @@
lower_packed_varyings_gs_splicer::visit_leave(ir_emit_vertex *ev)
return visit_continue;
}
+/**
+ * Visitor that splices varying packing code before every return in main().
+ */
+class lower_packed_varyings_return_splicer : public ir_hierarchical_visitor
+{
+public:
+ explicit lower_packed_varyings_return_splicer(void *mem_ctx,
+ const exec_list
*instructions);
+
+ virtual ir_visitor_status visit_leave(ir_return *ret);
+ virtual ir_visitor_status visit_enter(ir_function_signature *sig);
+
+private:
+ /**
+* Memory context used to allocate new instructions for the shader.
+*/
+ void * const mem_ctx;
+
+ /**
+* Instructions that should be spliced into place before each return.
+*/
+ const exec_list *instructions;
+};
+
+
+lower_packed_varyings_return_splicer::lower_packed_varyings_return_splicer(
+ void *mem_ctx, const exec_list *instructions)
+ : mem_ctx(mem_ctx), instructions(instructions)
+{
+}
+
+ir_visitor_status
+lower_packed_varyings_return_splicer::visit_leave(ir_return *ret)
+{
+ foreach_in_list(ir_instruction, ir, this->instructions) {
+ ret->insert_before(ir->clone(this->mem_ctx, NULL));
+ }
+ return visit_continue;
+}
+
+ir_visitor_status
+lower_packed_varyings_return_splicer::visit_enter(ir_function_signature *sig)
+{
+ /* Only splice returns into main(). */
+ if (!strcmp(sig->function_name(), "main"))
+ return visit_continue;
+ return visit_continue_with_parent;
+}
+
void
lower_packed_varyings(void *mem_ctx, unsigned locations_used,
@@ -757,11 +806,22 @@ lower_packed_varyings(void *mem_ctx, unsigned
locations_used,
/* Now update all the EmitVertex instances */
splicer.run(instructions);
} else {
- /* For other shader types, outputs need to be lowered at the end of
- * main()
+ /* For other shader types, outputs need to be lowered before each
+ * return statement and at the end of main()
+ */
+
+ lower_packed_varyings_return_splicer splicer(mem_ctx,
_instructions);
+
+ main_func_sig->body.head->insert_before(_variables);
+
+ splicer.run(instructions);
+
+ /* Lower outputs at the end of main() if the last instruction is not
+ * a return statement
*/
- main_func_sig->body.append_list(_variables);
- main_func_sig->body.append_list(_instructions);
+ if (((ir_instruction*)instructions->get_tail())->ir_type !=
ir_type_return) {
+main_func_sig->body.append_list(_instructions);
+ }
}
} else {
/* Shader inputs need to be lowered at the beginning of main() */
--
2.5.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: fix lowering outputs for early/nested returns
Hi Timothy,
Thanks for your comments, you're absolutely right about not wanting to
visit other functions.
I will modify the visitor appropriately and submit a couple of piglit tests.
Regards,
Lars Hamre
On Mon, Apr 25, 2016 at 8:20 PM, Timothy Arceri
<timothy.arc...@collabora.com> wrote:
> Hi Lars,
>
> Thankd for working on this, comments below.
>
> On Sun, 2016-04-17 at 13:18 -0400, Lars Hamre wrote:
>> Return statements in conditional blocks were not having their
>> output varyings lowered correctly.
>>
>> This patch fixes the following piglit tests:
>> /spec/glsl-1.10/execution/vs-float-main-return
>> /spec/glsl-1.10/execution/vs-vec2-main-return
>> /spec/glsl-1.10/execution/vs-vec3-main-return
>>
>> Signed-off-by: Lars Hamre <cheme...@gmail.com>
>>
>> ---
>>
>> NOTE: Someone with access will need to commit this after the review
>> process
>>
>> src/compiler/glsl/lower_packed_varyings.cpp | 58
>> +++--
>> 1 file changed, 54 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/compiler/glsl/lower_packed_varyings.cpp
>> b/src/compiler/glsl/lower_packed_varyings.cpp
>> index 825cc9e..41edada 100644
>> --- a/src/compiler/glsl/lower_packed_varyings.cpp
>> +++ b/src/compiler/glsl/lower_packed_varyings.cpp
>> @@ -724,6 +724,45 @@
>> lower_packed_varyings_gs_splicer::visit_leave(ir_emit_vertex *ev)
>> return visit_continue;
>> }
>>
>> +/**
>> + * Visitor that splices varying packing code before every return.
>> + */
>> +class lower_packed_varyings_return_splicer : public
>> ir_hierarchical_visitor
>> +{
>> +public:
>> + explicit lower_packed_varyings_return_splicer(void *mem_ctx,
>> + const exec_list
>> *instructions);
>> +
>> + virtual ir_visitor_status visit_leave(ir_return *ret);
>> +
>> +private:
>> + /**
>> +* Memory context used to allocate new instructions for the
>> shader.
>> +*/
>> + void * const mem_ctx;
>> +
>> + /**
>> +* Instructions that should be spliced into place before each
>> return.
>> +*/
>> + const exec_list *instructions;
>> +};
>> +
>> +
>> +lower_packed_varyings_return_splicer::lower_packed_varyings_return_s
>> plicer(
>> + void *mem_ctx, const exec_list *instructions)
>> + : mem_ctx(mem_ctx), instructions(instructions)
>> +{
>> +}
>> +
>> +
>> +ir_visitor_status
>> +lower_packed_varyings_return_splicer::visit_leave(ir_return *ret)
>> +{
>> + foreach_in_list(ir_instruction, ir, this->instructions) {
>> + ret->insert_before(ir->clone(this->mem_ctx, NULL));
>> + }
>> + return visit_continue;
>> +}
>>
>> void
>> lower_packed_varyings(void *mem_ctx, unsigned locations_used,
>> @@ -757,11 +796,22 @@ lower_packed_varyings(void *mem_ctx, unsigned
>> locations_used,
>> /* Now update all the EmitVertex instances */
>> splicer.run(instructions);
>>} else {
>> - /* For other shader types, outputs need to be lowered at
>> the end of
>> - * main()
>> + /* For other shader types, outputs need to be lowered
>> before each
>> + * return statement and at the end of main()
>>*/
>> - main_func_sig->body.append_list(_variables);
>> - main_func_sig->body.append_list(_instructions);
>> +
>> + lower_packed_varyings_return_splicer splicer(mem_ctx,
>> _instructions);
>> +
>> + main_func_sig->body.head->insert_before(_variables);
>> +
>> + splicer.run(instructions);
>
> This will walk over everything. I don't think you want that you only
> want to walk over the instructions in main right?
>
> For example if you have another function with a return then you would
> end up inserting the instructions before that functions return too. You
> could probably create a piglit test to detect this.
>
>
>> +
>> + /* Lower outputs at the end of main() if the last
>> instruction is not
>> + * a return statement
>> + */
>> + if (((ir_instruction*)instructions->get_tail())->ir_type !=
>> ir_type_return) {
>> +main_func_sig->body.append_list(_instructions);
>> + }
>
> Will this work for:
>
>
> foo1 = vec2(0.5);
> if (early_return != 0) {
> foo1 = vec2(0.2);
> return;
> }
>
> I assume it will be fine, but would be good to have a piglit test for
> this also.
>
>>}
>> } else {
>>/* Shader inputs need to be lowered at the beginning of main()
>> */
>> --
>> 2.5.5
>>
>> ___
>> 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
Re: [Mesa-dev] [PATCH] glsl: fix lowering outputs for early/nested returns
Gentle ping, I would appreciate feedback.
Regards,
Lars Hamre
On Sun, Apr 17, 2016 at 1:18 PM, Lars Hamre <cheme...@gmail.com> wrote:
> Return statements in conditional blocks were not having their
> output varyings lowered correctly.
>
> This patch fixes the following piglit tests:
> /spec/glsl-1.10/execution/vs-float-main-return
> /spec/glsl-1.10/execution/vs-vec2-main-return
> /spec/glsl-1.10/execution/vs-vec3-main-return
>
> Signed-off-by: Lars Hamre <cheme...@gmail.com>
>
> ---
>
> NOTE: Someone with access will need to commit this after the review
> process
>
> src/compiler/glsl/lower_packed_varyings.cpp | 58
> +++--
> 1 file changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/src/compiler/glsl/lower_packed_varyings.cpp
> b/src/compiler/glsl/lower_packed_varyings.cpp
> index 825cc9e..41edada 100644
> --- a/src/compiler/glsl/lower_packed_varyings.cpp
> +++ b/src/compiler/glsl/lower_packed_varyings.cpp
> @@ -724,6 +724,45 @@
> lower_packed_varyings_gs_splicer::visit_leave(ir_emit_vertex *ev)
> return visit_continue;
> }
>
> +/**
> + * Visitor that splices varying packing code before every return.
> + */
> +class lower_packed_varyings_return_splicer : public ir_hierarchical_visitor
> +{
> +public:
> + explicit lower_packed_varyings_return_splicer(void *mem_ctx,
> + const exec_list
> *instructions);
> +
> + virtual ir_visitor_status visit_leave(ir_return *ret);
> +
> +private:
> + /**
> +* Memory context used to allocate new instructions for the shader.
> +*/
> + void * const mem_ctx;
> +
> + /**
> +* Instructions that should be spliced into place before each return.
> +*/
> + const exec_list *instructions;
> +};
> +
> +
> +lower_packed_varyings_return_splicer::lower_packed_varyings_return_splicer(
> + void *mem_ctx, const exec_list *instructions)
> + : mem_ctx(mem_ctx), instructions(instructions)
> +{
> +}
> +
> +
> +ir_visitor_status
> +lower_packed_varyings_return_splicer::visit_leave(ir_return *ret)
> +{
> + foreach_in_list(ir_instruction, ir, this->instructions) {
> + ret->insert_before(ir->clone(this->mem_ctx, NULL));
> + }
> + return visit_continue;
> +}
>
> void
> lower_packed_varyings(void *mem_ctx, unsigned locations_used,
> @@ -757,11 +796,22 @@ lower_packed_varyings(void *mem_ctx, unsigned
> locations_used,
> /* Now update all the EmitVertex instances */
> splicer.run(instructions);
>} else {
> - /* For other shader types, outputs need to be lowered at the end of
> - * main()
> + /* For other shader types, outputs need to be lowered before each
> + * return statement and at the end of main()
>*/
> - main_func_sig->body.append_list(_variables);
> - main_func_sig->body.append_list(_instructions);
> +
> + lower_packed_varyings_return_splicer splicer(mem_ctx,
> _instructions);
> +
> + main_func_sig->body.head->insert_before(_variables);
> +
> + splicer.run(instructions);
> +
> + /* Lower outputs at the end of main() if the last instruction is not
> + * a return statement
> + */
> + if (((ir_instruction*)instructions->get_tail())->ir_type !=
> ir_type_return) {
> +main_func_sig->body.append_list(_instructions);
> + }
>}
> } else {
>/* Shader inputs need to be lowered at the beginning of main() */
> --
> 2.5.5
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: Relax GLSL 1.10 float suffix error to a warning.
I am fine with this, it would be nice if we could modify the following
piglit tests to pass when a warning is emitted:
https://cgit.freedesktop.org/piglit/tree/tests/spec/glsl-1.10/compiler/literals/invalid-float-suffix-capital-f.vert
https://cgit.freedesktop.org/piglit/tree/tests/spec/glsl-1.10/compiler/literals/invalid-float-suffix-f.vert
Either way,
Reviewed-by: Lars Hamre <cheme...@gmail.com>
On Wed, Apr 20, 2016 at 3:29 PM, Matt Turner <matts...@gmail.com> wrote:
>
> Float suffixes are allowed in all subsequent GLSL specifications, and
> it's obvious what the user meant if they specify one. Accept it with a
> warning to avoid breaking applications, like Planeshift.
> ---
> src/compiler/glsl/glsl_lexer.ll | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/compiler/glsl/glsl_lexer.ll b/src/compiler/glsl/glsl_lexer.ll
> index 6b1ef17..8a562cb 100644
> --- a/src/compiler/glsl/glsl_lexer.ll
> +++ b/src/compiler/glsl/glsl_lexer.ll
> @@ -476,8 +476,8 @@ layout {
> char suffix = yytext[strlen(yytext) - 1];
> if (!state->is_version(120, 300) &&
> (suffix == 'f' || suffix == 'F')) {
> - _mesa_glsl_error(yylloc, state,
> -"Float suffixes are invalid
> in GLSL 1.10");
> + _mesa_glsl_warning(yylloc, state,
> + "Float suffixes are
> invalid in GLSL 1.10");
> }
> yylval->real = _mesa_strtof(yytext, NULL);
> return FLOATCONSTANT;
> --
> 2.7.3
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] glsl: fix lowering outputs for early/nested returns
Return statements in conditional blocks were not having their
output varyings lowered correctly.
This patch fixes the following piglit tests:
/spec/glsl-1.10/execution/vs-float-main-return
/spec/glsl-1.10/execution/vs-vec2-main-return
/spec/glsl-1.10/execution/vs-vec3-main-return
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
NOTE: Someone with access will need to commit this after the review
process
src/compiler/glsl/lower_packed_varyings.cpp | 58 +++--
1 file changed, 54 insertions(+), 4 deletions(-)
diff --git a/src/compiler/glsl/lower_packed_varyings.cpp
b/src/compiler/glsl/lower_packed_varyings.cpp
index 825cc9e..41edada 100644
--- a/src/compiler/glsl/lower_packed_varyings.cpp
+++ b/src/compiler/glsl/lower_packed_varyings.cpp
@@ -724,6 +724,45 @@
lower_packed_varyings_gs_splicer::visit_leave(ir_emit_vertex *ev)
return visit_continue;
}
+/**
+ * Visitor that splices varying packing code before every return.
+ */
+class lower_packed_varyings_return_splicer : public ir_hierarchical_visitor
+{
+public:
+ explicit lower_packed_varyings_return_splicer(void *mem_ctx,
+ const exec_list
*instructions);
+
+ virtual ir_visitor_status visit_leave(ir_return *ret);
+
+private:
+ /**
+* Memory context used to allocate new instructions for the shader.
+*/
+ void * const mem_ctx;
+
+ /**
+* Instructions that should be spliced into place before each return.
+*/
+ const exec_list *instructions;
+};
+
+
+lower_packed_varyings_return_splicer::lower_packed_varyings_return_splicer(
+ void *mem_ctx, const exec_list *instructions)
+ : mem_ctx(mem_ctx), instructions(instructions)
+{
+}
+
+
+ir_visitor_status
+lower_packed_varyings_return_splicer::visit_leave(ir_return *ret)
+{
+ foreach_in_list(ir_instruction, ir, this->instructions) {
+ ret->insert_before(ir->clone(this->mem_ctx, NULL));
+ }
+ return visit_continue;
+}
void
lower_packed_varyings(void *mem_ctx, unsigned locations_used,
@@ -757,11 +796,22 @@ lower_packed_varyings(void *mem_ctx, unsigned
locations_used,
/* Now update all the EmitVertex instances */
splicer.run(instructions);
} else {
- /* For other shader types, outputs need to be lowered at the end of
- * main()
+ /* For other shader types, outputs need to be lowered before each
+ * return statement and at the end of main()
*/
- main_func_sig->body.append_list(_variables);
- main_func_sig->body.append_list(_instructions);
+
+ lower_packed_varyings_return_splicer splicer(mem_ctx,
_instructions);
+
+ main_func_sig->body.head->insert_before(_variables);
+
+ splicer.run(instructions);
+
+ /* Lower outputs at the end of main() if the last instruction is not
+ * a return statement
+ */
+ if (((ir_instruction*)instructions->get_tail())->ir_type !=
ir_type_return) {
+main_func_sig->body.append_list(_instructions);
+ }
}
} else {
/* Shader inputs need to be lowered at the beginning of main() */
--
2.5.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2] glsl: handle unsigned int wraparound in link_shaders()
NOTE: someone with access will need to commit this afer the
review process
v2: change check_explicit_uniform_locations() to return an
unsigned 0 (Timothy Arceri)
We were storing the int result of check_explicit_uniform_locations()
in num_explicit_uniform_locs as an unsigned int which caused it to
be 4294967295 when a -1 was returned.
This in turn would cause the following error during linking:
error: count of uniform locations > MAX_UNIFORM_LOCATIONS(4294967295 > 98304)
Here are the results from running piglit tests/all with this patch:
changes: 178
fixes: 176
regressions: 2
The two regressions are for the following tests:
glean@glsl1-matrix column check (1)
glean@glsl1-matrix column check (2)
which regress from FAIL to CRASH.
I am okay with these regressions because the tests are currently
failing due to the aforementioned linker error.
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
src/compiler/glsl/linker.cpp | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
index cd35464..cc74574 100644
--- a/src/compiler/glsl/linker.cpp
+++ b/src/compiler/glsl/linker.cpp
@@ -3164,12 +3164,12 @@ reserve_subroutine_explicit_locations(struct
gl_shader_program *prog,
* any optimizations happen to handle also inactive uniforms and
* inactive array elements that may get trimmed away.
*/
-static int
+static unsigned
check_explicit_uniform_locations(struct gl_context *ctx,
struct gl_shader_program *prog)
{
if (!ctx->Extensions.ARB_explicit_uniform_location)
- return -1;
+ return 0;
/* This map is used to detect if overlapping explicit locations
* occur with the same uniform (from different stage) or a different one.
@@ -3178,7 +3178,7 @@ check_explicit_uniform_locations(struct gl_context *ctx,
if (!uniform_map) {
linker_error(prog, "Out of memory during linking.\n");
- return -1;
+ return 0;
}
unsigned entries_total = 0;
@@ -3207,7 +3207,7 @@ check_explicit_uniform_locations(struct gl_context *ctx,
}
if (!ret) {
delete uniform_map;
- return -1;
+ return 0;
}
}
}
--
2.5.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: handle unsigned int wraparound in link_shaders()
Agreed, I didn't see that check_explicit_uniform_locations() was only used in link_shaders(). I will submit a v2 with those changes. On Thu, Apr 7, 2016 at 11:24 PM, Timothy Arceri < timothy.arc...@collabora.com> wrote: > On Thu, 2016-04-07 at 11:22 -0400, Lars Hamre wrote: > > NOTES: > > * Reposting due to no response last week > > * Someone with access will need to commit this patch after the review > > process > > > > check_explicit_uniform_locations() returns a -1 when the extension > > ARB_explicit_uniform_location is not found. > > > > We were storing the result in num_explicit_uniform_locs as an > > unsigned int which caused it to be 4294967295 when a -1 was returned. > > > > This in turn would cause the following error during linking: > > error: count of uniform locations > MAX_UNIFORM_LOCATIONS(4294967295 > > > 98304) > > > > Here are the results from running piglit tests/all with this patch: > > changes: 178 > > fixes: 176 > > regressions: 2 > > > > The two regressions are for the following tests: > > glean@glsl1-matrix column check (1) > > glean@glsl1-matrix column check (2) > > which regress from FAIL to CRASH. > > > > I am okay with these regressions because the tests are currently > > failing due to the aforementioned linker error. > > > > Signed-off-by: Lars Hamre <cheme...@gmail.com> > > > > --- > > src/compiler/glsl/linker.cpp | 6 -- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/src/compiler/glsl/linker.cpp > > b/src/compiler/glsl/linker.cpp > > index cd35464..a0a9104 100644 > > --- a/src/compiler/glsl/linker.cpp > > +++ b/src/compiler/glsl/linker.cpp > > @@ -4171,7 +4171,7 @@ link_shaders(struct gl_context *ctx, struct > > gl_shader_program *prog) > > > > tfeedback_decl *tfeedback_decls = NULL; > > unsigned num_tfeedback_decls = prog- > > >TransformFeedback.NumVarying; > > - unsigned int num_explicit_uniform_locs = 0; > > + int num_explicit_uniform_locs = 0; > > > > void *mem_ctx = ralloc_context(NULL); // temporary linker context > > > > @@ -4354,6 +4354,8 @@ link_shaders(struct gl_context *ctx, struct > > gl_shader_program *prog) > > } > > > > num_explicit_uniform_locs = check_explicit_uniform_locations(ctx, > > prog); > > + if (num_explicit_uniform_locs < 0) > > + num_explicit_uniform_locs = 0; > > Thanks for spotting this. However the just hacks around the problem. > > Since we never do anything with the -1 I would rather > see check_explicit_uniform_locations() changed to return an unsigned 0 > rather than -1. > > > > link_assign_subroutine_types(prog); > > > > if (!prog->LinkStatus) > > @@ -4585,7 +4587,7 @@ link_shaders(struct gl_context *ctx, struct > > gl_shader_program *prog) > > > > update_array_sizes(prog); > > link_assign_uniform_locations(prog, ctx- > > >Const.UniformBooleanTrue, > > - num_explicit_uniform_locs, > > + (unsigned > > int)num_explicit_uniform_locs, > > ctx- > > >Const.MaxUserAssignableUniformLocations); > > link_assign_atomic_counter_resources(ctx, prog); > > store_fragdepth_layout(prog); > > -- > > 2.5.5 > > > > ___ > > 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] glsl: handle unsigned int wraparound in link_shaders()
NOTES: * Reposting due to no response last week * Someone with access will need to commit this patch after the review process check_explicit_uniform_locations() returns a -1 when the extension ARB_explicit_uniform_location is not found. We were storing the result in num_explicit_uniform_locs as an unsigned int which caused it to be 4294967295 when a -1 was returned. This in turn would cause the following error during linking: error: count of uniform locations > MAX_UNIFORM_LOCATIONS(4294967295 > 98304) Here are the results from running piglit tests/all with this patch: changes: 178 fixes: 176 regressions: 2 The two regressions are for the following tests: glean@glsl1-matrix column check (1) glean@glsl1-matrix column check (2) which regress from FAIL to CRASH. I am okay with these regressions because the tests are currently failing due to the aforementioned linker error. Signed-off-by: Lars Hamre <cheme...@gmail.com> --- src/compiler/glsl/linker.cpp | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp index cd35464..a0a9104 100644 --- a/src/compiler/glsl/linker.cpp +++ b/src/compiler/glsl/linker.cpp @@ -4171,7 +4171,7 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) tfeedback_decl *tfeedback_decls = NULL; unsigned num_tfeedback_decls = prog->TransformFeedback.NumVarying; - unsigned int num_explicit_uniform_locs = 0; + int num_explicit_uniform_locs = 0; void *mem_ctx = ralloc_context(NULL); // temporary linker context @@ -4354,6 +4354,8 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) } num_explicit_uniform_locs = check_explicit_uniform_locations(ctx, prog); + if (num_explicit_uniform_locs < 0) + num_explicit_uniform_locs = 0; link_assign_subroutine_types(prog); if (!prog->LinkStatus) @@ -4585,7 +4587,7 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) update_array_sizes(prog); link_assign_uniform_locations(prog, ctx->Const.UniformBooleanTrue, - num_explicit_uniform_locs, + (unsigned int)num_explicit_uniform_locs, ctx->Const.MaxUserAssignableUniformLocations); link_assign_atomic_counter_resources(ctx, prog); store_fragdepth_layout(prog); -- 2.5.5 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3] compiler/glsl: invalidate float suffixes for glsl 1.10
Hi Ian and Roland,
I am new to mesa development, so please bare with my questions:
- Is there some protocol for testing against the private shader-db?
(I assume private means I don't have access)
- Would the person checking PlaneShift be someone who owns the
game and can test it against mesa master?
- If we were to change to solely emitting an error, would we remove
the piglit tests in question or just accept the failures?
I am in agreement with Ian about preferring the driconf option so
we could adhere to spec; however, Roland brings up an
important point that this could break other applications which
potentially use the float suffix.
Thanks for the feedback.
Regards,
Lars
On Thu, Mar 31, 2016 at 10:35 AM, Roland Scheidegger <srol...@vmware.com>
wrote:
> Am 31.03.2016 um 04:17 schrieb Ian Romanick:
> > Arg. :(
> >
> > It turns out that shaders in the game PlaneShift use float suffixes in
> > GLSL 1.10 shaders... at least the version of the shaders in our private
> > shader-db repo do. Once some one check to make sure the game still does
> > this, we will need some sort of patch.
> >
> > I see two likely options.
> >
> > 1. Just change the error to a warning.
> >
> > 2. Add a driconf option that changes the error to a warning. Enable
> > this option for PlaneShift in the default .drirc.
> >
> > I'd prefer option #2, personally.
>
>
> I wouldn't be surprised if other apps did the same thing. Forbidding
> float suffixes sounds sort of unusual (and unexpected) to me. Albeit
> there weren't really any other data types then, so possibly noone else
> did it...
>
> Roland
>
> >
> > On 03/28/2016 05:42 PM, Lars Hamre wrote:
> >> NOTE: someone with access will need to commit this patch after the
> >> review process
> >>
> >> v2: modify error message
> >> v3: parse the float instead of returning an ERROR_TOK
> >>
> >> Invalidates float suffixes for glsl 1.10
> >>
> >> Fixes the following piglit tests:
> >>
> tests/spec/glsl-1.10/compiler/literals/invalid-float-suffix-capital-f.vert
> >> tests/spec/glsl-1.10/compiler/literals/invalid-float-suffix-f.vert`
> >>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81585
> >>
> >> Signed-off-by: Lars Hamre <cheme...@gmail.com>
> >> Reviewed-by: Timothy Arceri <timothy.arc...@collabora.com>
> >> Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
> >>
> >> ---
> >> src/compiler/glsl/glsl_lexer.ll | 7 +++
> >> 1 file changed, 7 insertions(+)
> >>
> >> diff --git a/src/compiler/glsl/glsl_lexer.ll
> b/src/compiler/glsl/glsl_lexer.ll
> >> index 1f12265..6084d71 100644
> >> --- a/src/compiler/glsl/glsl_lexer.ll
> >> +++ b/src/compiler/glsl/glsl_lexer.ll
> >> @@ -472,6 +472,13 @@ layout {
> >> \.[0-9]+([eE][+-]?[0-9]+)?[fF]? |
> >> [0-9]+\.([eE][+-]?[0-9]+)?[fF]? |
> >> [0-9]+[eE][+-]?[0-9]+[fF]? {
> >> +struct _mesa_glsl_parse_state *state = yyextra;
> >> +char suffix = yytext[strlen(yytext) - 1];
> >> +if (!state->is_version(120, 0) &&
> >> +(suffix == 'f' || suffix == 'F')) {
> >> +_mesa_glsl_error(yylloc, state,
> >> + "Float suffixes are
> invalid in GLSL 1.10");
> >> +}
> >> yylval->real = _mesa_strtof(yytext, NULL);
> >> return FLOATCONSTANT;
> >> }
> >> --
> >> 2.5.5
> >>
> >> ___
> >> 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 mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] glsl: handle unsigned int wraparound in link_shaders()
check_explicit_uniform_locations() returns a -1 when the extension ARB_explicit_uniform_location is not found. We were storing the result in num_explicit_uniform_locs as an unsigned int which caused it to be 4294967295 when a -1 was returned. This in turn would cause the following error during linking: error: count of uniform locations > MAX_UNIFORM_LOCATIONS(4294967295 > 98304) Here are the results from running piglit tests/all with this patch: changes: 178 fixes: 176 regressions: 2 The two regressions are for the following tests: glean@glsl1-matrix column check (1) glean@glsl1-matrix column check (2) which regress from FAIL to CRASH. I am okay with these regressions because the tests are currently failing due to the aforementioned linker error. Signed-off-by: Lars Hamre <cheme...@gmail.com> --- src/compiler/glsl/linker.cpp | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp index cd35464..a0a9104 100644 --- a/src/compiler/glsl/linker.cpp +++ b/src/compiler/glsl/linker.cpp @@ -4171,7 +4171,7 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) tfeedback_decl *tfeedback_decls = NULL; unsigned num_tfeedback_decls = prog->TransformFeedback.NumVarying; - unsigned int num_explicit_uniform_locs = 0; + int num_explicit_uniform_locs = 0; void *mem_ctx = ralloc_context(NULL); // temporary linker context @@ -4354,6 +4354,8 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) } num_explicit_uniform_locs = check_explicit_uniform_locations(ctx, prog); + if (num_explicit_uniform_locs < 0) + num_explicit_uniform_locs = 0; link_assign_subroutine_types(prog); if (!prog->LinkStatus) @@ -4585,7 +4587,7 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) update_array_sizes(prog); link_assign_uniform_locations(prog, ctx->Const.UniformBooleanTrue, - num_explicit_uniform_locs, + (unsigned int)num_explicit_uniform_locs, ctx->Const.MaxUserAssignableUniformLocations); link_assign_atomic_counter_resources(ctx, prog); store_fragdepth_layout(prog); -- 2.5.5 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v3] compiler/glsl: invalidate float suffixes for glsl 1.10
NOTE: someone with access will need to commit this patch after the
review process
v2: modify error message
v3: parse the float instead of returning an ERROR_TOK
Invalidates float suffixes for glsl 1.10
Fixes the following piglit tests:
tests/spec/glsl-1.10/compiler/literals/invalid-float-suffix-capital-f.vert
tests/spec/glsl-1.10/compiler/literals/invalid-float-suffix-f.vert`
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81585
Signed-off-by: Lars Hamre <cheme...@gmail.com>
Reviewed-by: Timothy Arceri <timothy.arc...@collabora.com>
Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
---
src/compiler/glsl/glsl_lexer.ll | 7 +++
1 file changed, 7 insertions(+)
diff --git a/src/compiler/glsl/glsl_lexer.ll b/src/compiler/glsl/glsl_lexer.ll
index 1f12265..6084d71 100644
--- a/src/compiler/glsl/glsl_lexer.ll
+++ b/src/compiler/glsl/glsl_lexer.ll
@@ -472,6 +472,13 @@ layout {
\.[0-9]+([eE][+-]?[0-9]+)?[fF]?|
[0-9]+\.([eE][+-]?[0-9]+)?[fF]?|
[0-9]+[eE][+-]?[0-9]+[fF]? {
+ struct _mesa_glsl_parse_state *state = yyextra;
+ char suffix = yytext[strlen(yytext) - 1];
+ if (!state->is_version(120, 0) &&
+ (suffix == 'f' || suffix == 'F')) {
+ _mesa_glsl_error(yylloc, state,
+"Float suffixes are invalid in
GLSL 1.10");
+ }
yylval->real = _mesa_strtof(yytext, NULL);
return FLOATCONSTANT;
}
--
2.5.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] compiler/glsl: invalidate float suffixes for glsl 1.10
That sounds good to me, I will submit a v3 with those edits.
On Mon, Mar 28, 2016 at 9:19 PM, Kenneth Graunke <kenn...@whitecape.org>
wrote:
> On Monday, March 28, 2016 8:16:17 PM PDT Lars Hamre wrote:
> > NOTE: someone with access will need to commit this patch after the
> > review process
> >
> > Invalidates float suffixes for glsl 1.10
> >
> > Fixes the following piglit tests:
> >
> tests/spec/glsl-1.10/compiler/literals/invalid-float-suffix-capital-f.vert
> > tests/spec/glsl-1.10/compiler/literals/invalid-float-suffix-f.vert`
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81585
> >
> > Signed-off-by: Lars Hamre <cheme...@gmail.com>
> > Reviewed-by: Timothy Arceri <timothy.arc...@collabora.com>
> >
> > ---
> > src/compiler/glsl/glsl_lexer.ll | 13 +++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/compiler/glsl/glsl_lexer.ll b/src/compiler/glsl/
> glsl_lexer.ll
> > index 1f12265..0980f4f 100644
> > --- a/src/compiler/glsl/glsl_lexer.ll
> > +++ b/src/compiler/glsl/glsl_lexer.ll
> > @@ -472,8 +472,17 @@ layout {
> > \.[0-9]+([eE][+-]?[0-9]+)?[fF]? |
> > [0-9]+\.([eE][+-]?[0-9]+)?[fF]? |
> > [0-9]+[eE][+-]?[0-9]+[fF]? {
> > - yylval->real = _mesa_strtof(yytext, NULL);
> > - return FLOATCONSTANT;
> > + struct _mesa_glsl_parse_state *state = yyextra;
> > + char suffix = yytext[strlen(yytext) - 1];
> > + if (!state->is_version(120, 0) &&
> > + (suffix == 'f' || suffix == 'F')) {
> > + _mesa_glsl_error(yylloc, state,
> > + "Float suffixes are
> invalid in GLSL
> 1.10");
> > + return ERROR_TOK;
> > + } else {
> > + yylval->real = _mesa_strtof(yytext, NULL);
> > + return FLOATCONSTANT;
> > + }
>
> Hi Lars,
>
> Good catch! Would it also work to do:
>
> if (!state->is_version(120, 0) &&
> (suffix == 'f' || suffix == 'F')) {
> _mesa_glsl_error(yylloc, state,
> "Float suffixes are invalid in
> GLSL 1.10");
> }
> yylval->real = _mesa_strtof(yytext, NULL);
> return FLOATCONSTANT;
>
> In other words, raise the error and fail the compile, but parse the
> float literal as intended. I think returning ERROR_TOK is likely to
> make the parser hit bison-generated "Expected , got OTHER TOKENS>" errors that are pretty cryptic. Parsing the number in
> the obvious manner would let the compile proceed normally, so we don't
> get cascading errors that might be confusing.
>
> That patch would get a:
> Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
>
> > }
> >
> > [0-9]+\.[0-9]+([eE][+-]?[0-9]+)?(lf|LF) |
> > --
> > 2.5.5
> >
> > ___
> > 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 v2] compiler/glsl: invalidate float suffixes for glsl 1.10
NOTE: someone with access will need to commit this patch after the
review process
Invalidates float suffixes for glsl 1.10
Fixes the following piglit tests:
tests/spec/glsl-1.10/compiler/literals/invalid-float-suffix-capital-f.vert
tests/spec/glsl-1.10/compiler/literals/invalid-float-suffix-f.vert`
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81585
Signed-off-by: Lars Hamre <cheme...@gmail.com>
Reviewed-by: Timothy Arceri <timothy.arc...@collabora.com>
---
src/compiler/glsl/glsl_lexer.ll | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/compiler/glsl/glsl_lexer.ll b/src/compiler/glsl/glsl_lexer.ll
index 1f12265..0980f4f 100644
--- a/src/compiler/glsl/glsl_lexer.ll
+++ b/src/compiler/glsl/glsl_lexer.ll
@@ -472,8 +472,17 @@ layout {
\.[0-9]+([eE][+-]?[0-9]+)?[fF]?|
[0-9]+\.([eE][+-]?[0-9]+)?[fF]?|
[0-9]+[eE][+-]?[0-9]+[fF]? {
- yylval->real = _mesa_strtof(yytext, NULL);
- return FLOATCONSTANT;
+ struct _mesa_glsl_parse_state *state = yyextra;
+ char suffix = yytext[strlen(yytext) - 1];
+ if (!state->is_version(120, 0) &&
+ (suffix == 'f' || suffix == 'F')) {
+ _mesa_glsl_error(yylloc, state,
+"Float suffixes are invalid in
GLSL 1.10");
+ return ERROR_TOK;
+ } else {
+ yylval->real = _mesa_strtof(yytext, NULL);
+ return FLOATCONSTANT;
+ }
}
[0-9]+\.[0-9]+([eE][+-]?[0-9]+)?(lf|LF)|
--
2.5.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] compiler/glsl: invalidate float suffixes for glsl 1.10
Invalidates float suffixes for glsl 1.10
Fixes the following piglit tests:
tests/spec/glsl-1.10/compiler/literals/invalid-float-suffix-capital-f.vert
tests/spec/glsl-1.10/compiler/literals/invalid-float-suffix-f.vert`
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
src/compiler/glsl/glsl_lexer.ll | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/compiler/glsl/glsl_lexer.ll b/src/compiler/glsl/glsl_lexer.ll
index 1f12265..dc71009 100644
--- a/src/compiler/glsl/glsl_lexer.ll
+++ b/src/compiler/glsl/glsl_lexer.ll
@@ -472,8 +472,17 @@ layout {
\.[0-9]+([eE][+-]?[0-9]+)?[fF]?|
[0-9]+\.([eE][+-]?[0-9]+)?[fF]?|
[0-9]+[eE][+-]?[0-9]+[fF]? {
- yylval->real = _mesa_strtof(yytext, NULL);
- return FLOATCONSTANT;
+ struct _mesa_glsl_parse_state *state = yyextra;
+ char suffix = yytext[strlen(yytext) - 1];
+ if (!state->is_version(120, 0) &&
+ (suffix == 'f' || suffix == 'F')) {
+ _mesa_glsl_error(yylloc, state,
+"Float suffixes are invalid");
+ return ERROR_TOK;
+ } else {
+ yylval->real = _mesa_strtof(yytext, NULL);
+ return FLOATCONSTANT;
+ }
}
[0-9]+\.[0-9]+([eE][+-]?[0-9]+)?(lf|LF)|
--
2.5.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3] compiler/glsl: allow sequence op as a const expr in gles 1.0
Thanks! Will do in the future.
Regards,
Lars
On Wed, Mar 23, 2016 at 1:08 PM, Eduardo Lima Mitev <el...@igalia.com>
wrote:
> Hi Lars,
>
> I suppose you need help pushing your patch upstream. I will do it.
>
> As a note for the future, once you get a formal Review-by to a patch, it
> is not necessary to send a new version only to add the tag. You can instead
> ask directly someone with committing rights to merge it for you.
>
> cheers,
> Eduardo
>
>
> On 03/23/2016 03:14 PM, Lars Hamre wrote:
>
>> v3: Added reviewed-by tag
>> v2: Fixed regression pointed out by Eduardo Lima Mitev
>>
>> Allow the sequence operator to be a constant expression in GLSL ES
>> versions prior
>> to GLSL ES 3.0
>>
>> Fixes the following piglit test:
>>
>> /all/spec/glsl-es-1.0/compiler/array-sized-by-sequence-in-parenthesis.vert
>>
>> This is similar to the logic from process_initializer() which performs the
>> same check for constant variable initialization with sequence operators.
>>
>> Signed-off-by: Lars Hamre <cheme...@gmail.com>
>> Reviewed-by: Eduardo Lima Mitev <el...@igalia.com>
>>
>> ---
>> src/compiler/glsl/ast_to_hir.cpp | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/compiler/glsl/ast_to_hir.cpp
>> b/src/compiler/glsl/ast_to_hir.cpp
>> index 5262bd8..35def8e 100644
>> --- a/src/compiler/glsl/ast_to_hir.cpp
>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>> @@ -2125,7 +2125,9 @@ process_array_size(exec_node *node,
>> }
>>
>> ir_constant *const size = ir->constant_expression_value();
>> - if (size == NULL || array_size->has_sequence_subexpression()) {
>> + if (size == NULL ||
>> + (state->is_version(120, 300) &&
>> +array_size->has_sequence_subexpression())) {
>> _mesa_glsl_error(& loc, state, "array size must be a "
>> "constant valued expression");
>> return 0;
>> --
>> 2.5.0
>>
>> ___
>> 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 v3] compiler/glsl: allow sequence op as a const expr in gles 1.0
v3: Added reviewed-by tag
v2: Fixed regression pointed out by Eduardo Lima Mitev
Allow the sequence operator to be a constant expression in GLSL ES versions
prior
to GLSL ES 3.0
Fixes the following piglit test:
/all/spec/glsl-es-1.0/compiler/array-sized-by-sequence-in-parenthesis.vert
This is similar to the logic from process_initializer() which performs the
same check for constant variable initialization with sequence operators.
Signed-off-by: Lars Hamre <cheme...@gmail.com>
Reviewed-by: Eduardo Lima Mitev <el...@igalia.com>
---
src/compiler/glsl/ast_to_hir.cpp | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
index 5262bd8..35def8e 100644
--- a/src/compiler/glsl/ast_to_hir.cpp
+++ b/src/compiler/glsl/ast_to_hir.cpp
@@ -2125,7 +2125,9 @@ process_array_size(exec_node *node,
}
ir_constant *const size = ir->constant_expression_value();
- if (size == NULL || array_size->has_sequence_subexpression()) {
+ if (size == NULL ||
+ (state->is_version(120, 300) &&
+array_size->has_sequence_subexpression())) {
_mesa_glsl_error(& loc, state, "array size must be a "
"constant valued expression");
return 0;
--
2.5.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2] compiler/glsl: allow sequence op as const expression for gles 1.0
v2: Fixed regression pointed out by Eduardo Lima Mitev
Allow the sequence operator to be a constant expression in GLSL ES versions
prior
to GLSL ES 3.0
Fixes the following piglit test:
/all/spec/glsl-es-1.0/compiler/array-sized-by-sequence-in-parenthesis.vert
This is similar to the logic from process_initializer() which performs the
same check for constant variable initialization with sequence operators.
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
src/compiler/glsl/ast_to_hir.cpp | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
index 5262bd8..35def8e 100644
--- a/src/compiler/glsl/ast_to_hir.cpp
+++ b/src/compiler/glsl/ast_to_hir.cpp
@@ -2125,7 +2125,9 @@ process_array_size(exec_node *node,
}
ir_constant *const size = ir->constant_expression_value();
- if (size == NULL || array_size->has_sequence_subexpression()) {
+ if (size == NULL ||
+ (state->is_version(120, 300) &&
+array_size->has_sequence_subexpression())) {
_mesa_glsl_error(& loc, state, "array size must be a "
"constant valued expression");
return 0;
--
2.5.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] compiler/glsl: Allow the sequence operator to be a constant expression
You are correct, it should be state->is_version(120, 300).
I will submit an updated patch.
On Tue, Mar 22, 2016 at 3:32 PM, Eduardo Lima Mitev <el...@igalia.com>
wrote:
> On 03/22/2016 02:48 PM, Lars Hamre wrote:
>
>> Resending this patch because it received no response last week.
>>
>> Allow the sequence operator to be a constant expression in GLSL ES
>> versions prior
>> to GLSL ES 3.0
>>
>> Fixes the following piglit test:
>>
>> /all/spec/glsl-es-1.0/compiler/array-sized-by-sequence-in-parenthesis.vert
>>
>>
> I confirm this fixes the above test, but it also regresses test:
>
>
> /all/spec/glsl-1.20/compiler/structure-and-array-operations/array-size-sequence-in-parenthesis.vert.
>
> Maybe you are missing a version check?
>
> Eduardo
>
> This mirrors the logic from process_initializer() which performs the
>> same check for constant variable initialization with sequence operators.
>>
>> Section 4.3.3 (Constant Expressions) of the GLSL 4.30.9 spec and of the
>> GLSL ES 3.00.4 spec say that the result of a sequence operator is not a
>> constant expression; however, we should not mandate that for lower GLSL
>> versions.
>>
>> Signed-off-by: Lars Hamre <cheme...@gmail.com>
>>
>> ---
>> src/compiler/glsl/ast_to_hir.cpp | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/compiler/glsl/ast_to_hir.cpp
>> b/src/compiler/glsl/ast_to_hir.cpp
>> index 5262bd8..4037468 100644
>> --- a/src/compiler/glsl/ast_to_hir.cpp
>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>> @@ -2125,7 +2125,9 @@ process_array_size(exec_node *node,
>> }
>>
>> ir_constant *const size = ir->constant_expression_value();
>> - if (size == NULL || array_size->has_sequence_subexpression()) {
>> + if (size == NULL ||
>> + (state->is_version(430, 300) &&
>> +array_size->has_sequence_subexpression())) {
>> _mesa_glsl_error(& loc, state, "array size must be a "
>> "constant valued expression");
>> return 0;
>> --
>> 2.5.0
>>
>> ___
>> 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] compiler/glsl: Allow the sequence operator to be a constant expression
Resending this patch because it received no response last week.
Allow the sequence operator to be a constant expression in GLSL ES versions
prior
to GLSL ES 3.0
Fixes the following piglit test:
/all/spec/glsl-es-1.0/compiler/array-sized-by-sequence-in-parenthesis.vert
This mirrors the logic from process_initializer() which performs the
same check for constant variable initialization with sequence operators.
Section 4.3.3 (Constant Expressions) of the GLSL 4.30.9 spec and of the
GLSL ES 3.00.4 spec say that the result of a sequence operator is not a
constant expression; however, we should not mandate that for lower GLSL
versions.
Signed-off-by: Lars Hamre <cheme...@gmail.com>
---
src/compiler/glsl/ast_to_hir.cpp | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
index 5262bd8..4037468 100644
--- a/src/compiler/glsl/ast_to_hir.cpp
+++ b/src/compiler/glsl/ast_to_hir.cpp
@@ -2125,7 +2125,9 @@ process_array_size(exec_node *node,
}
ir_constant *const size = ir->constant_expression_value();
- if (size == NULL || array_size->has_sequence_subexpression()) {
+ if (size == NULL ||
+ (state->is_version(430, 300) &&
+array_size->has_sequence_subexpression())) {
_mesa_glsl_error(& loc, state, "array size must be a "
"constant valued expression");
return 0;
--
2.5.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] compiler/glsl: Allow the sequence operator to be a constant expression in GLSL ES versions prior to GLSL ES 3.0
Allow the sequence operator to be a constant expression in GLSL ES versions
prior
to GLSL ES 3.0
Fixes the following piglit test:
/all/spec/glsl-es-1.0/compiler/array-sized-by-sequence-in-parenthesis.vert
This mirrors the logic from process_initializer() which performs the
same check for constant variable initialization with sequence operators.
Section 4.3.3 (Constant Expressions) of the GLSL 4.30.9 spec and of the
GLSL ES 3.00.4 spec say that the result of a sequence operator is not a
constant expression; however, we should not mandate that for lower GLSL
versions.
---
src/compiler/glsl/ast_to_hir.cpp | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
index 5262bd8..4037468 100644
--- a/src/compiler/glsl/ast_to_hir.cpp
+++ b/src/compiler/glsl/ast_to_hir.cpp
@@ -2125,7 +2125,9 @@ process_array_size(exec_node *node,
}
ir_constant *const size = ir->constant_expression_value();
- if (size == NULL || array_size->has_sequence_subexpression()) {
+ if (size == NULL ||
+ (state->is_version(430, 300) &&
+array_size->has_sequence_subexpression())) {
_mesa_glsl_error(& loc, state, "array size must be a "
"constant valued expression");
return 0;
--
2.5.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] swrast: fix possible null dereference
Sounds good, is there a protocol to signify a patch isn't active anymore? On 03/09/2016 04:37 PM, Lars Hamre wrote: > I have not been able to force a NULL dereference, this is based off > analyzing the code. > Yes that is implicitly true, but if at some point the implicit > relationship is broken, I would > rather not have a NULL dereference. > > If you do not agree, I am fine deferring to your judgement! > I don't think the code in question will be hit if texObj is null. This code is pretty old (perhaps 15 years) but has been used a lot. I think we'd have known by now if there was a null pointer problem. -Brian > On Wed, Mar 9, 2016 at 6:23 PM, Ian Romanick <i...@freedesktop.org > <mailto:i...@freedesktop.org>> wrote: > > On 03/09/2016 10:21 AM, Lars Hamre wrote: > > Fixes a possible null dereference. > > > > NOTE: this is my first time contributing, please let me know if I > > should be doing anything differently, thanks! > > > > Signed-off-by: Lars Hamre <cheme...@gmail.com > <mailto:cheme...@gmail.com>> > > --- > > src/mesa/swrast/s_triangle.c | 7 --- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/src/mesa/swrast/s_triangle.c > b/src/mesa/swrast/s_triangle.c > > index 876a74b..9225974 100644 > > --- a/src/mesa/swrast/s_triangle.c > > +++ b/src/mesa/swrast/s_triangle.c > > @@ -781,7 +781,7 @@ fast_persp_span(struct gl_context *ctx, > SWspan *span, > >} > >break; > > } > > - > > + > > assert(span->arrayMask & SPAN_RGBA); > > _swrast_write_rgba_span(ctx, span); > > > > @@ -1063,8 +1063,8 @@ _swrast_choose_triangle( struct gl_context > *ctx ) > > swImg = swrast_texture_image_const(texImg); > > > > format = texImg ? texImg->TexFormat : MESA_FORMAT_NONE; > > - minFilter = texObj2D ? samp->MinFilter : GL_NONE; > > - magFilter = texObj2D ? samp->MagFilter : GL_NONE; > > + minFilter = (texObj2D && samp) ? samp->MinFilter : > GL_NONE; > > + magFilter = (texObj2D && samp) ? samp->MagFilter : > GL_NONE; > > NAK this hunk. If texObj2D is not NULL, samp is also not NULL. > > > envMode = ctx->Texture.Unit[0].EnvMode; > > > > /* First see if we can use an optimized 2-D texture > function */ > > @@ -1073,6 +1073,7 @@ _swrast_choose_triangle( struct gl_context > *ctx ) > > && !ctx->ATIFragmentShader._Enabled > > && ctx->Texture._MaxEnabledTexImageUnit == 0 > > && ctx->Texture.Unit[0]._Current->Target == > GL_TEXTURE_2D > > + && samp > > I think the 'ctx->Texture.Unit[0]._Current->Target == GL_TEXTURE_2D' > implicitly ensures that samp cannot be NULL. Have you been able to > cause a NULL dereference in this code path or is this just based on > speculation? > > > && samp->WrapS == GL_REPEAT > > && samp->WrapT == GL_REPEAT > > && texObj2D->_Swizzle == SWIZZLE_NOOP > > -- > > 2.5.0 > > > > ___ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > <mailto: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 mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] swrast: fix possible null dereference
I have not been able to force a NULL dereference, this is based off analyzing the code. Yes that is implicitly true, but if at some point the implicit relationship is broken, I would rather not have a NULL dereference. If you do not agree, I am fine deferring to your judgement! On Wed, Mar 9, 2016 at 6:23 PM, Ian Romanick <i...@freedesktop.org> wrote: > On 03/09/2016 10:21 AM, Lars Hamre wrote: > > Fixes a possible null dereference. > > > > NOTE: this is my first time contributing, please let me know if I > > should be doing anything differently, thanks! > > > > Signed-off-by: Lars Hamre <cheme...@gmail.com> > > --- > > src/mesa/swrast/s_triangle.c | 7 --- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/src/mesa/swrast/s_triangle.c b/src/mesa/swrast/s_triangle.c > > index 876a74b..9225974 100644 > > --- a/src/mesa/swrast/s_triangle.c > > +++ b/src/mesa/swrast/s_triangle.c > > @@ -781,7 +781,7 @@ fast_persp_span(struct gl_context *ctx, SWspan *span, > >} > >break; > > } > > - > > + > > assert(span->arrayMask & SPAN_RGBA); > > _swrast_write_rgba_span(ctx, span); > > > > @@ -1063,8 +1063,8 @@ _swrast_choose_triangle( struct gl_context *ctx ) > > swImg = swrast_texture_image_const(texImg); > > > > format = texImg ? texImg->TexFormat : MESA_FORMAT_NONE; > > - minFilter = texObj2D ? samp->MinFilter : GL_NONE; > > - magFilter = texObj2D ? samp->MagFilter : GL_NONE; > > + minFilter = (texObj2D && samp) ? samp->MinFilter : GL_NONE; > > + magFilter = (texObj2D && samp) ? samp->MagFilter : GL_NONE; > > NAK this hunk. If texObj2D is not NULL, samp is also not NULL. > > > envMode = ctx->Texture.Unit[0].EnvMode; > > > > /* First see if we can use an optimized 2-D texture function */ > > @@ -1073,6 +1073,7 @@ _swrast_choose_triangle( struct gl_context *ctx ) > > && !ctx->ATIFragmentShader._Enabled > > && ctx->Texture._MaxEnabledTexImageUnit == 0 > > && ctx->Texture.Unit[0]._Current->Target == GL_TEXTURE_2D > > + && samp > > I think the 'ctx->Texture.Unit[0]._Current->Target == GL_TEXTURE_2D' > implicitly ensures that samp cannot be NULL. Have you been able to > cause a NULL dereference in this code path or is this just based on > speculation? > > > && samp->WrapS == GL_REPEAT > > && samp->WrapT == GL_REPEAT > > && texObj2D->_Swizzle == SWIZZLE_NOOP > > -- > > 2.5.0 > > > > ___ > > 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] swrast: fix possible null dereference
Fixes a possible null dereference. NOTE: this is my first time contributing, please let me know if I should be doing anything differently, thanks! Signed-off-by: Lars Hamre <cheme...@gmail.com> --- src/mesa/swrast/s_triangle.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/mesa/swrast/s_triangle.c b/src/mesa/swrast/s_triangle.c index 876a74b..9225974 100644 --- a/src/mesa/swrast/s_triangle.c +++ b/src/mesa/swrast/s_triangle.c @@ -781,7 +781,7 @@ fast_persp_span(struct gl_context *ctx, SWspan *span, } break; } - + assert(span->arrayMask & SPAN_RGBA); _swrast_write_rgba_span(ctx, span); @@ -1063,8 +1063,8 @@ _swrast_choose_triangle( struct gl_context *ctx ) swImg = swrast_texture_image_const(texImg); format = texImg ? texImg->TexFormat : MESA_FORMAT_NONE; - minFilter = texObj2D ? samp->MinFilter : GL_NONE; - magFilter = texObj2D ? samp->MagFilter : GL_NONE; + minFilter = (texObj2D && samp) ? samp->MinFilter : GL_NONE; + magFilter = (texObj2D && samp) ? samp->MagFilter : GL_NONE; envMode = ctx->Texture.Unit[0].EnvMode; /* First see if we can use an optimized 2-D texture function */ @@ -1073,6 +1073,7 @@ _swrast_choose_triangle( struct gl_context *ctx ) && !ctx->ATIFragmentShader._Enabled && ctx->Texture._MaxEnabledTexImageUnit == 0 && ctx->Texture.Unit[0]._Current->Target == GL_TEXTURE_2D + && samp && samp->WrapS == GL_REPEAT && samp->WrapT == GL_REPEAT && texObj2D->_Swizzle == SWIZZLE_NOOP -- 2.5.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
