On Wednesday, October 21, 2015 03:58:15 PM Matt Turner wrote:
> Initially just checks that sources are non-NULL, which would have
> alerted us to the problem fixed by commit 6c846dc5.
> ---
> src/mesa/drivers/dri/i965/Makefile.sources | 1 +
> src/mesa/drivers/dri/i965/brw_eu.h | 4 +
> src/mesa/drivers/dri/i965/brw_eu_validate.c | 150
> +++++++++++++++++++++++
> src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 8 ++
> src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 8 ++
> 5 files changed, 171 insertions(+)
> create mode 100644 src/mesa/drivers/dri/i965/brw_eu_validate.c
>
> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources
> b/src/mesa/drivers/dri/i965/Makefile.sources
> index c2438bd..7cd9cc0 100644
> --- a/src/mesa/drivers/dri/i965/Makefile.sources
> +++ b/src/mesa/drivers/dri/i965/Makefile.sources
> @@ -14,6 +14,7 @@ i965_compiler_FILES = \
> brw_eu_emit.c \
> brw_eu.h \
> brw_eu_util.c \
> + brw_eu_validate.c \
> brw_fs_builder.h \
> brw_fs_channel_expressions.cpp \
> brw_fs_cmod_propagation.cpp \
> diff --git a/src/mesa/drivers/dri/i965/brw_eu.h
> b/src/mesa/drivers/dri/i965/brw_eu.h
> index 1345db7..829e393 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu.h
> +++ b/src/mesa/drivers/dri/i965/brw_eu.h
> @@ -522,6 +522,10 @@ bool brw_try_compact_instruction(const struct
> brw_device_info *devinfo,
> void brw_debug_compact_uncompact(const struct brw_device_info *devinfo,
> brw_inst *orig, brw_inst *uncompacted);
>
> +/* brw_eu_validate.c */
> +bool brw_validate_instructions(const struct brw_codegen *p, int start_offset,
> + struct annotation_info *annotation);
> +
> static inline int
> next_offset(const struct brw_device_info *devinfo, void *store, int offset)
> {
> diff --git a/src/mesa/drivers/dri/i965/brw_eu_validate.c
> b/src/mesa/drivers/dri/i965/brw_eu_validate.c
> new file mode 100644
> index 0000000..85d4c19
> --- /dev/null
> +++ b/src/mesa/drivers/dri/i965/brw_eu_validate.c
> @@ -0,0 +1,150 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +/** @file brw_eu_validate.c
> + *
> + * This file implements a pass that validates shader assembly.
> + */
> +
> +#include "brw_eu.h"
> +
> +/* We're going to do lots of string concatenation, so this should help. */
> +struct string {
> + char *str;
> + size_t len;
> +};
> +
> +static void
> +cat(struct string *dest, const struct string src)
> +{
> + dest->str = realloc(dest->str, dest->len + src.len + 1);
> + memcpy(dest->str + dest->len, src.str, src.len);
> + dest->str[dest->len + src.len + 1] = '\0';
> + dest->len = dest->len + src.len;
> +}
> +#define CAT(dest, src) cat(&dest, (struct string){src, strlen(src)})
> +
> +#define error(str) "\tERROR: " str "\n"
> +
> +#define ERROR_IF(cond, msg) \
> + do { \
> + if (cond) { \
> + CAT(error_msg, error(msg)); \
> + valid = false; \
> + } \
> + } while(0)
Are you going to want to support printf-style messages someday?
This infrastructure won't really work for that...error() only handles
string literals...
I don't see why you wouldn't just do:
#define ERROR_F_IF(cond, fmt, ...) \
do { \
if (cond) { \
ralloc_asprintf_rewrite_tail(&error_msg.str, &error_msg.len, \
"\tERROR: " fmt "\n", __VA_ARGS__); \
valid = false; \
} \
} while(0)
#define ERROR_IF(cond, msg) ERROR_F_IF(cond, "%s", msg)
Then ralloc_free(error_msg.str) later instead of free().
It's more flexible and avoids the need for cat(), CAT(), and error().
> +
> +static bool
> +src0_is_null(const struct brw_device_info *devinfo, const brw_inst *inst)
> +{
> + return brw_inst_src0_reg_file(devinfo, inst) ==
> BRW_ARCHITECTURE_REGISTER_FILE &&
> + brw_inst_src0_da_reg_nr(devinfo, inst) == BRW_ARF_NULL;
> +}
> +
> +static bool
> +src1_is_null(const struct brw_device_info *devinfo, const brw_inst *inst)
> +{
> + return brw_inst_src1_reg_file(devinfo, inst) ==
> BRW_ARCHITECTURE_REGISTER_FILE &&
> + brw_inst_src1_da_reg_nr(devinfo, inst) == BRW_ARF_NULL;
> +}
> +
> +static unsigned
> +num_sources_from_inst(const struct brw_device_info *devinfo,
> + const brw_inst *inst)
> +{
> + unsigned math_function;
> +
> + if (brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MATH) {
> + math_function = brw_inst_math_function(devinfo, inst);
> + } else if (devinfo->gen < 6 &&
> + brw_inst_opcode(devinfo, inst) == BRW_OPCODE_SEND) {
> + if (brw_inst_sfid(devinfo, inst) == BRW_SFID_MATH) {
> + math_function = brw_inst_math_msg_function(devinfo, inst);
> + } else {
> + return 0;
Technically, Gen4-5 SEND instructions can have 1 source, which is
implicitly MOV'd to inst->base_mrf...but I guess you don't want to
handle that here, as the source is optional (so ARF NULL is OK).
I hope you like my suggestions, but either way...
Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
> + }
> + } else {
> + return opcode_descs[brw_inst_opcode(devinfo, inst)].nsrc;
> + }
> +
> + switch (math_function) {
> + case BRW_MATH_FUNCTION_INV:
> + case BRW_MATH_FUNCTION_LOG:
> + case BRW_MATH_FUNCTION_EXP:
> + case BRW_MATH_FUNCTION_SQRT:
> + case BRW_MATH_FUNCTION_RSQ:
> + case BRW_MATH_FUNCTION_SIN:
> + case BRW_MATH_FUNCTION_COS:
> + case BRW_MATH_FUNCTION_SINCOS:
> + case GEN8_MATH_FUNCTION_INVM:
> + case GEN8_MATH_FUNCTION_RSQRTM:
> + return 1;
> + case BRW_MATH_FUNCTION_FDIV:
> + case BRW_MATH_FUNCTION_POW:
> + case BRW_MATH_FUNCTION_INT_DIV_QUOTIENT_AND_REMAINDER:
> + case BRW_MATH_FUNCTION_INT_DIV_QUOTIENT:
> + case BRW_MATH_FUNCTION_INT_DIV_REMAINDER:
> + return 2;
> + default:
> + unreachable("not reached");
> + }
> +}
> +
> +bool
> +brw_validate_instructions(const struct brw_codegen *p, int start_offset,
> + struct annotation_info *annotation)
> +{
> + const struct brw_device_info *devinfo = p->devinfo;
> + const void *store = p->store + start_offset / 16;
> + bool valid = true;
> +
> + for (int src_offset = 0; src_offset < p->next_insn_offset - start_offset;
> + src_offset += sizeof(brw_inst)) {
> + struct string error_msg = { .str = NULL, .len = 0 };
> + const brw_inst *inst = store + src_offset;
> +
> + switch (num_sources_from_inst(devinfo, inst)) {
> + case 3:
> + /* Nothing to test. 3-src instructions can only have GRF sources,
> and
> + * there's no bit to control the file.
> + */
> + break;
> + case 2:
> + ERROR_IF(src1_is_null(devinfo, inst), "src1 is null");
> + /* fallthrough */
> + case 1:
> + ERROR_IF(src0_is_null(devinfo, inst), "src0 is null");
> + break;
> + case 0:
> + default:
> + break;
> + }
> +
> + if (error_msg.str && annotation) {
> + annotation_insert_error(annotation, src_offset, error_msg.str);
> + }
> + free(error_msg.str);
> + }
> +
> + return valid;
> +}
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> index 8ab57f7..80a6a4c 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> @@ -2172,6 +2172,13 @@ fs_generator::generate_code(const cfg_t *cfg, int
> dispatch_width)
> brw_set_uip_jip(p);
> annotation_finalize(&annotation, p->next_insn_offset);
>
> +#ifndef NDEBUG
> + bool validated = brw_validate_instructions(p, start_offset, &annotation);
> +#else
> + if (unlikely(debug_flag))
> + brw_validate_instructions(p, start_offset, &annotation);
> +#endif
> +
> int before_size = p->next_insn_offset - start_offset;
> brw_compact_instructions(p, start_offset, annotation.ann_count,
> annotation.ann);
> @@ -2189,6 +2196,7 @@ fs_generator::generate_code(const cfg_t *cfg, int
> dispatch_width)
> p->devinfo);
> ralloc_free(annotation.mem_ctx);
> }
> + assert(validated);
>
> compiler->shader_debug_log(log_data,
> "%s SIMD%d shader: %d inst, %d loops, "
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> index 6ac8591..88deb53 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> @@ -1642,6 +1642,13 @@ vec4_generator::generate_code(const cfg_t *cfg, const
> nir_shader *nir)
> brw_set_uip_jip(p);
> annotation_finalize(&annotation, p->next_insn_offset);
>
> +#ifndef NDEBUG
> + bool validated = brw_validate_instructions(p, 0, &annotation);
> +#else
> + if (unlikely(debug_flag))
> + brw_validate_instructions(p, 0, &annotation);
> +#endif
> +
> int before_size = p->next_insn_offset;
> brw_compact_instructions(p, 0, annotation.ann_count, annotation.ann);
> int after_size = p->next_insn_offset;
> @@ -1661,6 +1668,7 @@ vec4_generator::generate_code(const cfg_t *cfg, const
> nir_shader *nir)
> p->devinfo);
> ralloc_free(annotation.mem_ctx);
> }
> + assert(validated);
>
> compiler->shader_debug_log(log_data,
> "%s vec4 shader: %d inst, %d loops, "
>
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
