[Mesa-dev] [PATCH 7/9] i965: Add initial assembly validation pass.
Matt Turner
mattst88 at gmail.com
Tue Nov 3 22:23:48 PST 2015
On Tue, Nov 3, 2015 at 10:14 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> 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...
Hmm. I'm not sure. Seems like it might be useful, but I haven't
encountered a case where I want it yet.
>
> 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().
That might be good.
>> +
>> +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).
Yeah, exactly.
> I hope you like my suggestions, but either way...
>
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
Thanks!
More information about the mesa-dev
mailing list