[Mesa-dev] [PATCH] i965/fs: Add a very basic validation pass
Matt Turner
mattst88 at gmail.com
Thu Jul 30 15:53:18 PDT 2015
On Thu, Jul 30, 2015 at 3:42 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> Currently the validation pass only validates that regs_read and
> regs_written are consistent with the sizes of VGRF's. We can add more as
> we find it to be useful.
> ---
> src/mesa/drivers/dri/i965/Makefile.sources | 1 +
> src/mesa/drivers/dri/i965/brw_fs.cpp | 9 ++++
> src/mesa/drivers/dri/i965/brw_fs.h | 1 +
> src/mesa/drivers/dri/i965/brw_fs_validate.cpp | 59 +++++++++++++++++++++++++++
> 4 files changed, 70 insertions(+)
> create mode 100644 src/mesa/drivers/dri/i965/brw_fs_validate.cpp
>
> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources b/src/mesa/drivers/dri/i965/Makefile.sources
> index e861c2c..26d19b8 100644
> --- a/src/mesa/drivers/dri/i965/Makefile.sources
> +++ b/src/mesa/drivers/dri/i965/Makefile.sources
> @@ -62,6 +62,7 @@ i965_FILES = \
> brw_fs_sel_peephole.cpp \
> brw_fs_surface_builder.cpp \
> brw_fs_surface_builder.h \
> + brw_fs_validate.cpp \
> brw_fs_vector_splitting.cpp \
> brw_fs_visitor.cpp \
> brw_gs.c \
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 15fe364..7127991 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -4664,6 +4664,9 @@ fs_visitor::calculate_register_pressure()
> void
> fs_visitor::optimize()
> {
> + /* Start by validating the shader we currently have. */
> + validate();
> +
> /* bld is the common builder object pointing at the end of the program we
> * used to translate it into i965 IR. For the optimization and lowering
> * passes coming next, any code added after the end of the program without
> @@ -4683,6 +4686,8 @@ fs_visitor::optimize()
> assign_constant_locations();
> demote_pull_constants();
>
> + validate();
> +
> #define OPT(pass, args...) ({ \
> pass_num++; \
> bool this_progress = pass(args); \
> @@ -4695,6 +4700,8 @@ fs_visitor::optimize()
> backend_shader::dump_instructions(filename); \
> } \
> \
> + validate(); \
> + \
> progress = progress || this_progress; \
> this_progress; \
> })
> @@ -4756,6 +4763,8 @@ fs_visitor::optimize()
> OPT(lower_integer_multiplication);
>
> lower_uniform_pull_constant_loads();
> +
> + validate();
> }
>
> /**
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> index 4749c47..e532e79 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -153,6 +153,7 @@ public:
> void invalidate_live_intervals();
> void calculate_live_intervals();
> void calculate_register_pressure();
> + void validate();
> bool opt_algebraic();
> bool opt_redundant_discard_jumps();
> bool opt_cse();
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_validate.cpp b/src/mesa/drivers/dri/i965/brw_fs_validate.cpp
> new file mode 100644
> index 0000000..cd65a58
> --- /dev/null
> +++ b/src/mesa/drivers/dri/i965/brw_fs_validate.cpp
> @@ -0,0 +1,59 @@
> +/*
> + * Copyright © 2012 Intel Corporation
2015
> + *
> + * 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_fs_validate.cpp
> + *
> + * Implements a pass that validates various invariants of the IR. The current
> + * pass only validates that GRF's uses are sane. More can be added later.
> + */
> +
> +#include "brw_fs.h"
> +#include "brw_cfg.h"
> +
> +#define fsv_assert(cond) ({ \
> + if (!(cond)) { \
> + fprintf(stderr, "ASSERT: FS validation failed!\n"); \
> + v->dump_instruction(inst, stderr); \
> + fprintf(stderr, "%s:%d: %s\n", __FILE__, __LINE__, #cond); \
> + abort(); \
> + } \
> +})
I don't think you need statement expressions here. I'm not actually
sure how this works. I thought you had to end statement expressions
with an expression -- at least that's the intention.
> +
> +void
> +fs_visitor::validate()
> +{
> + /* Needed for the fsv_assert macro */
> + fs_visitor *v = this;
Well, not really? Could just remove the v-> from the macro, I think.
> +
> + foreach_block_and_inst (block, fs_inst, inst, cfg) {
> + if (inst->dst.file == GRF) {
> + fsv_assert(inst->dst.reg_offset + inst->regs_written <= alloc.sizes[inst->dst.reg]);
Line wrap.
> + }
> +
> + for (unsigned i = 0; i < inst->sources; i++) {
> + if (inst->src[i].file == GRF) {
> + fsv_assert(inst->src[i].reg_offset + inst->regs_read(i) <= (int)alloc.sizes[inst->src[i].reg]);
Line wrap.
I noticed the (int) cast and checked whether regs_read() actually
needs to return a negative value, and it doesn't. You might want to
change regs_read() to return unsigned and remove the cast.
Reviewed-by: Matt Turner <mattst88 at gmail.com>
More information about the mesa-dev
mailing list