[Mesa-dev] [PATCH v6 2/6] mesa/st: glsl_to_tgsi: implement new temporary register lifetime tracker
Nicolai Hähnle
nhaehnle at gmail.com
Sun Jul 16 18:20:15 UTC 2017
On 04.07.2017 16:18, Gert Wollny wrote:
> This patch adds a class for tracking the life times of temporary registers
> in the glsl to tgsi translation. The algorithm runs in three steps:
> First, in order to minimize the number of needed memory allocations the
> program is scanned to evaluate the number of scopes.
> Then, the program is scanned second time to recorc the important register
*record
> access time points: first and last reads and writes and their link to the
> execution scope (loop, if/else branch, switch case).
> In the third step for each register the actuall minimal life time is
*actual
> evaluated.
>
> In addition, when compiled in debug mode (i.e. NDEBUG is not defined)
> the shaders and estimated temporary life times can be logged to stderr
> by setting the environment variable MESA_DEBUG_GLSL_TO_TGSI_RENAME.
Please make sure that the env variable name in the commit message
matches that in the code.
> ---
> src/mesa/Makefile.sources | 2 +
> .../state_tracker/st_glsl_to_tgsi_temprename.cpp | 949 +++++++++++++++++++++
> .../state_tracker/st_glsl_to_tgsi_temprename.h | 55 ++
> 3 files changed, 1006 insertions(+)
> create mode 100644 src/mesa/state_tracker/st_glsl_to_tgsi_temprename.cpp
> create mode 100644 src/mesa/state_tracker/st_glsl_to_tgsi_temprename.h
>
> diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
> index 5ec4e2d9a2..f1effdb8c1 100644
> --- a/src/mesa/Makefile.sources
> +++ b/src/mesa/Makefile.sources
> @@ -510,6 +510,8 @@ STATETRACKER_FILES = \
> state_tracker/st_glsl_to_tgsi.h \
> state_tracker/st_glsl_to_tgsi_private.cpp \
> state_tracker/st_glsl_to_tgsi_private.h \
> + state_tracker/st_glsl_to_tgsi_temprename.cpp \
> + state_tracker/st_glsl_to_tgsi_temprename.h \
> state_tracker/st_glsl_types.cpp \
> state_tracker/st_glsl_types.h \
> state_tracker/st_manager.c \
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi_temprename.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi_temprename.cpp
> new file mode 100644
> index 0000000000..f85a6fa7c9
> --- /dev/null
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi_temprename.cpp
> @@ -0,0 +1,949 @@
> +/*
> + * Copyright © 2017 Gert Wollny
> + *
> + * 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.
> + */
> +
> +#include "st_glsl_to_tgsi_temprename.h"
> +#include <tgsi/tgsi_info.h>
> +#include <tgsi/tgsi_strings.h>
> +#include <mesa/program/prog_instruction.h>
The mesa/ should be unnecessary and dropped for consistency.
> +#include <limits>
> +#include <cstdlib>
> +
> +/* std::sort is significantly faster than qsort */
> +#define USE_STL_SORT
> +#ifdef USE_STL_SORT
> +#include <algorithm>
> +#endif
> +
> +#ifndef NDEBUG
> +#include <iostream>
> +#include <iomanip>
> +using std::cerr;
> +using std::setw;
> +#endif
> +
> +using std::numeric_limits;
> +
> +/* Without c++11 define the nullptr for forward-compatibility
> + * and better readibility */
> +#if __cplusplus < 201103L
> +#define nullptr 0
> +#endif
> +
> +#ifndef NDEBUG
> +/* Helper class to check whether we want to seen debugging output */
> +class debug_log {
> +public:
> + static bool is_enabled();
> +private:
> + debug_log();
> + bool enabled;
> +};
It seems like a good idea to wrap a big part of this file in an
anonymous namespace. At least the class definitions.
One more comment about the debug_log: as written, it will getenv every
time. It would be better to use the approach taken by e.g.
should_clone_nir() in compiler/nir/nir.h. There's no reason to have a
whole class for this.
> +#define RENAME_DEBUG(X) if (debug_log::is_enabled()) do { X; } while (false);
> +#else
> +#define RENAME_DEBUG(X)
> +#endif
> +
> +enum prog_scope_type {
> + outer_scope, /* Outer program scope */
> + loop_body, /* Inside a loop */
> + if_branch, /* Inside if branch */
> + else_branch, /* Inside else branch */
> + switch_body, /* Inside switch statmenet */
> + switch_case_branch, /* Inside switch case statmenet */
> + switch_default_branch, /* Inside switch default statmenet */
> + undefined_scope
> +};
> +
> +class prog_scope {
> +public:
> + prog_scope(prog_scope *parent, prog_scope_type type, int id,
> + int depth, int begin);
> +
> + prog_scope_type type() const;
> + prog_scope *parent() const;
> + int nesting_depth() const;
> + int id() const;
> + int end() const;
> + int begin() const;
> + int loop_break_line() const;
> +
> + const prog_scope *in_ifelse_scope() const;
> + const prog_scope *in_switchcase_scope() const;
> + const prog_scope *innermost_loop() const;
> + const prog_scope *outermost_loop() const;
> + const prog_scope *enclosing_conditional() const;
> +
> + bool is_loop() const;
> + bool is_in_loop() const;
> + bool is_conditional() const;
> + bool is_conditional_in_loop() const;
> +
> + bool break_is_for_switchcase() const;
> + bool contains_range_of(const prog_scope& other) const;
> + const st_src_reg *switch_register() const;
> +
> + void set_end(int end);
> + void set_previous_case_scope(prog_scope *prev);
> + void set_loop_break_line(int line);
> + void set_switch_register(const st_src_reg *reg);
> +
> +private:
> + prog_scope_type scope_type;
> + int scope_id;
> + int scope_nesting_depth;
> + int scope_begin;
> + int scope_end;
> + int break_loop_line;
> + prog_scope *previous_case_scope;
> + prog_scope *parent_scope;
> + const st_src_reg *switch_reg;
> +};
> +
> +/* Some storage class to encapsulate the prog_scope (de-)allocations */
> +class prog_scope_storage {
> +public:
> + prog_scope_storage(void *mem_ctx, int n);
> + ~prog_scope_storage();
> + prog_scope * create(prog_scope *p, prog_scope_type type, int id,
> + int lvl, int s_begin);
> +private:
> + void *mem_ctx;
> + int current_slot;
> + prog_scope *storage;
> +};
> +
> +class temp_comp_access {
> +public:
> + temp_comp_access();
> + void record_read(int line, prog_scope *scope);
> + void record_write(int line, prog_scope *scope);
> + lifetime get_required_lifetime();
> +private:
> + void propagate_lifetime_to_dominant_write_scope();
> +
> + prog_scope *last_read_scope;
> + prog_scope *first_read_scope;
> + prog_scope *first_dominant_write_scope;
> + int first_dominant_write;
> + int last_read;
> + int last_write;
> + int first_read;
> + bool keep_for_full_loop;
> +};
> +
> +class temp_access {
> +public:
> + temp_access();
> + void record_read(int line, prog_scope *scope, int swizzle);
> + void record_write(int line, prog_scope *scope, int writemask);
> + lifetime get_required_lifetime();
> +private:
> + void update_access_mask(int mask);
> +
> + temp_comp_access comp[4];
> + int first_access_mask;
> + int summary_access_mask;
> + bool needs_component_tracking;
> +};
> +
> +#ifndef NDEBUG
> +/* function used for debugging. */
> +static void dump_instruction(int line, prog_scope *scope,
> + const glsl_to_tgsi_instruction& inst);
> +#endif
> +
> +/* Scan the program and estimate the required register life times.
> + * The array lifetimes must be pre-allocated
> + */
> +void
> +get_temp_registers_required_lifetimes(void *mem_ctx, exec_list *instructions,
> + int ntemps, struct lifetime *lifetimes)
> +{
> + int line = 0;
> + int loop_id = 0;
> + int if_id = 0;
> + int switch_id = 0;
> + bool is_at_end = false;
> + int n_scopes = 1;
> +
> +
> + /* count scopes to allocate the needed space without the need for
> + * re-allocation
> + */
> + foreach_in_list(glsl_to_tgsi_instruction, inst, instructions) {
> + if (inst->op == TGSI_OPCODE_BGNLOOP ||
> + inst->op == TGSI_OPCODE_SWITCH ||
> + inst->op == TGSI_OPCODE_CASE ||
> + inst->op == TGSI_OPCODE_IF ||
> + inst->op == TGSI_OPCODE_UIF ||
> + inst->op == TGSI_OPCODE_ELSE ||
> + inst->op == TGSI_OPCODE_DEFAULT)
> + ++n_scopes;
> + }
> +
> + prog_scope_storage scopes(mem_ctx, n_scopes);
> + temp_access *acc = new temp_access[ntemps];
> +
> + prog_scope *cur_scope = scopes.create(nullptr, outer_scope, 0, 0, line);
> +
> + RENAME_DEBUG(cerr << "========= Begin shader ============\n");
> +
> + foreach_in_list(glsl_to_tgsi_instruction, inst, instructions) {
> + if (is_at_end) {
> + assert(!"GLSL_TO_TGSI: shader has instructions past end marker");
> + break;
> + }
> +
> + RENAME_DEBUG(dump_instruction(line, cur_scope, *inst));
> +
> + switch (inst->op) {
> + case TGSI_OPCODE_BGNLOOP: {
> + cur_scope = scopes.create(cur_scope, loop_body, loop_id++,
> + cur_scope->nesting_depth()+1, line);
Spaces around operators, i.e.: cur_scope->nesting_depth() + 1.
> + break;
> + }
> + case TGSI_OPCODE_ENDLOOP: {
> + cur_scope->set_end(line);
> + cur_scope = cur_scope->parent();
> + assert(cur_scope);
> + break;
> + }
> + case TGSI_OPCODE_IF:
> + case TGSI_OPCODE_UIF: {
> + assert(num_inst_src_regs(inst) == 1);
> + const st_src_reg& src = inst->src[0];
> + if (src.file == PROGRAM_TEMPORARY)
> + acc[src.index].record_read(line, cur_scope, src.swizzle);
> + cur_scope = scopes.create(cur_scope, if_branch, if_id++,
> + cur_scope->nesting_depth()+1, line+1);
Spaces around operators.
> + break;
> + }
> + case TGSI_OPCODE_ELSE: {
> + assert(cur_scope->type() == if_branch);
> + cur_scope->set_end(line-1);
> + cur_scope = scopes.create(cur_scope->parent(), else_branch,
> + cur_scope->id(), cur_scope->nesting_depth(),
> + line+1);
Spaces around operators.
> + break;
> + }
> + case TGSI_OPCODE_END: {
> + cur_scope->set_end(line);
> + is_at_end = true;
> + break;
> + }
> + case TGSI_OPCODE_ENDIF: {
> + cur_scope->set_end(line-1);
> + cur_scope = cur_scope->parent();
> + assert(cur_scope);
> + break;
> + }
> + case TGSI_OPCODE_SWITCH: {
> + assert(num_inst_src_regs(inst) == 1);
> + const st_src_reg& src = inst->src[0];
> + prog_scope *scope = scopes.create(cur_scope, switch_body, switch_id++,
> + cur_scope->nesting_depth()+1, line);
Spaces around operators.
> + if (src.file == PROGRAM_TEMPORARY) {
> + acc[src.index].record_read(line, cur_scope, src.swizzle);
> + scope->set_switch_register(&src);
> + }
> + cur_scope = scope;
> + break;
> + }
> + case TGSI_OPCODE_ENDSWITCH: {
> + cur_scope->set_end(line-1);
Spaces around operators.
> + /* remove the case level, it might not have been
> + * closed with a break
As a general rule, comments should be capitalized properly.
> + */
> + if (cur_scope->type() != switch_body )
Extra space between if and (, and before the closing )
> + cur_scope = cur_scope->parent();
> +
> + cur_scope = cur_scope->parent();
> + assert(cur_scope);
> + break;
> + }
> + case TGSI_OPCODE_CASE: {
> + /* take care of tracking the registers */
Capitalize comment.
> + prog_scope *switch_scope = cur_scope->type() == switch_body ?
> + cur_scope : cur_scope->parent();
> +
> + assert(num_inst_src_regs(inst) == 1);
> + const st_src_reg& src = inst->src[0];
> + if (src.file == PROGRAM_TEMPORARY)
> + acc[src.index].record_read(line, switch_scope, src.swizzle);
I think this read (and the one of the switch_register) should both be on
the line of the SWITCH statement, so at the beginning of the switch_scope.
What you're doing is only more conservative, and I don't mind that much,
except that at least it should be possible to remove the
set_switch_register and friends, since a read of the switch register
need not be recorded for every CASE statement.
> +
> + /* we also must record the access to the register
Capitalize comment.
> + * specified with the switch
> + */
> + const st_src_reg *reg = switch_scope->switch_register();
> + if (reg)
> + acc[reg->index].record_read(line, switch_scope, reg->swizzle);
> +
> + /* fall through to allocate the scope */
I guess we can make an exception here since people often don't
capitalize fall-through...
> + }
> + case TGSI_OPCODE_DEFAULT: {
> + prog_scope_type t = inst->op == TGSI_OPCODE_CASE ? switch_case_branch
> + : switch_default_branch;
> + prog_scope *switch_scope = cur_scope;
> + if (cur_scope->type() == switch_body) {
> + cur_scope = scopes.create(cur_scope, t, cur_scope->id(),
> + cur_scope->nesting_depth()+1, line+1);
Spaces around operators.
> + } else {
> + switch_scope = cur_scope->parent();
> + assert(switch_scope->type() == switch_body);
> + prog_scope *scope = scopes.create(switch_scope, t,
> + switch_scope->id(),
> + switch_scope->nesting_depth()+1,
Spaces around operators.
> + line);
The two branches are inconsistent about where the new scope ends.
Overall, I think this could be simplified:
prog_scope *switch_scope =
cur_scope->type() == switch_body ? cur_scope : cur_scope->parent();
assert(switch_scope->type() == switch_body);
prog_scope *scope = scopes.create(...);
if (cur_scope != switch_body && cur_scope->end() == -1)
scope->set_previous_case_scope(cur_scope);
cur_scope = scope;
For that matter, I'm not sure the previous_case_scope handling is
necessary or whether it's correct at all. Do you really need the end to
overlap the next scope on fall-through? And if yes, what happens if have
multiple fall-throughs in a row?
> + /* Previous case falls through */
> + if (cur_scope->end() == -1)
> + scope->set_previous_case_scope(cur_scope);
> + cur_scope = scope;
> + }
> + break;
> + }
> + case TGSI_OPCODE_BRK: {
> + if (cur_scope->break_is_for_switchcase()) { > + cur_scope->set_end(line-1);
Spaces around operators.
> + } else {
> + cur_scope->set_loop_break_line(line);
> + }
> + break;
> + }
> + default: {
> + for (unsigned j = 0; j < num_inst_src_regs(inst); j++) {
> + const st_src_reg& src = inst->src[j];
> + if (src.file == PROGRAM_TEMPORARY)
> + acc[src.index].record_read(line, cur_scope, src.swizzle);
> + }
> + for (unsigned j = 0; j < inst->tex_offset_num_offset; j++) {
> + const st_src_reg& src = inst->tex_offsets[j];
> + if (src.file == PROGRAM_TEMPORARY)
> + acc[src.index].record_read(line, cur_scope, src.swizzle);
> + }
> + for (unsigned j = 0; j < num_inst_dst_regs(inst); j++) {
> + const st_dst_reg& dst = inst->dst[j];
> + if (dst.file == PROGRAM_TEMPORARY)
> + acc[dst.index].record_write(line, cur_scope, dst.writemask);
> + }
> + }
> + }
> + ++line;
> + }
> +
> + RENAME_DEBUG(cerr << "==================================\n\n");
> +
> + /* make sure last scope is closed, even though no
> + * TGSI_OPCODE_END was given
> + */
Capitalize comment.
> + if (cur_scope->end() < 0)
> + cur_scope->set_end(line-1);
Spaces around operators.
> +
> + RENAME_DEBUG(cerr << "========= lifetimes ==============\n");
> + for(int i = 1; i < ntemps; ++i) {
> + RENAME_DEBUG(cerr << setw(4) << i);
> + lifetimes[i] = acc[i].get_required_lifetime();
> + RENAME_DEBUG(cerr << ": [" << lifetimes[i].begin << ", "
> + << lifetimes[i].end << "]\n");
> + }
> + RENAME_DEBUG(cerr << "==================================\n\n");
> +
> + delete[] acc;
> +}
> +
> +prog_scope_storage::prog_scope_storage(void *mc, int n):
> + mem_ctx(mc),
> + current_slot(0)
> +{
> + storage = ralloc_array(mem_ctx, prog_scope, n);
> +}
> +
> +prog_scope_storage::~prog_scope_storage()
> +{
> + ralloc_free(storage);
> +}
> +
> +prog_scope*
> +prog_scope_storage::create(prog_scope *p, prog_scope_type type, int id,
> + int lvl, int s_begin)
> +{
> + storage[current_slot] = prog_scope(p, type, id, lvl, s_begin);
> + return &storage[current_slot++];
> +}
> +
> +prog_scope::prog_scope(prog_scope *parent, prog_scope_type type, int id,
> + int depth, int scope_begin):
> + scope_type(type),
> + scope_id(id),
> + scope_nesting_depth(depth),
> + scope_begin(scope_begin),
> + scope_end(-1),
> + break_loop_line(numeric_limits<int>::max()),
> + parent_scope(parent),
> + switch_reg(nullptr)
> +{
> +}
> +
> +prog_scope_type prog_scope::type() const
> +{
> + return scope_type;
> +}
> +
> +prog_scope *prog_scope::parent() const
> +{
> + return parent_scope;
> +}
> +
> +int prog_scope::nesting_depth() const
> +{
> + return scope_nesting_depth;
> +}
> +
> +bool prog_scope::is_loop() const
> +{
> + return (scope_type == loop_body);
> +}
> +
> +bool prog_scope::is_in_loop() const
> +{
> + if (scope_type == loop_body)
> + return true;
> +
> + if (parent_scope)
> + return parent_scope->is_in_loop();
> +
> + return false;
> +}
> +
> +bool prog_scope::is_conditional_in_loop() const
> +{
> + return is_conditional() && is_in_loop();
> +}
> +
> +const prog_scope *prog_scope::innermost_loop() const
> +{
> + if (scope_type == loop_body)
> + return this;
> +
> + if (parent_scope)
> + return parent_scope->innermost_loop();
> +
> + return nullptr;
> +}
> +
> +const prog_scope *prog_scope::outermost_loop() const
> +{
> + const prog_scope *loop = nullptr;
> + const prog_scope *p = this;
> +
> + do {
> + if (p->type() == loop_body)
> + loop = p;
> + p = p->parent();
> + } while (p);
> +
> + return loop;
> +}
> +
> +const prog_scope *prog_scope::enclosing_conditional() const
> +{
> + if (is_conditional())
> + return this;
> +
> + if (parent_scope)
> + return parent_scope->enclosing_conditional();
> +
> + return nullptr;
> +}
> +
> +bool prog_scope::contains_range_of(const prog_scope& other) const
> +{
> + return (begin() <= other.begin()) && (end() >= other.end());
> +}
> +
> +bool prog_scope::is_conditional() const
> +{
> + return scope_type == if_branch ||
> + scope_type == else_branch ||
> + scope_type == switch_case_branch ||
> + scope_type == switch_default_branch;
> +}
> +
> +const prog_scope *prog_scope::in_ifelse_scope() const
> +{
> + if (scope_type == if_branch ||
> + scope_type == else_branch)
> + return this;
> +
> + if (parent_scope)
> + return parent_scope->in_ifelse_scope();
> +
> + return nullptr;
> +}
> +
> +void prog_scope::set_switch_register(const st_src_reg *reg)
> +{
> + switch_reg = reg;
> +}
> +
> +const st_src_reg *prog_scope::switch_register() const
> +{
> + return switch_reg;
> +}
> +
> +const prog_scope *prog_scope::in_switchcase_scope() const
> +{
> + if (scope_type == switch_case_branch ||
> + scope_type == switch_default_branch)
> + return this;
> +
> + if (parent_scope)
> + return parent_scope->in_switchcase_scope();
> +
> + return nullptr;
> +}
> +
> +bool prog_scope::break_is_for_switchcase() const
> +{
> + if (scope_type == loop_body)
> + return false;
> +
> + if (scope_type == switch_case_branch ||
> + scope_type == switch_default_branch ||
> + scope_type == switch_body)
> + return true;
> +
> + if (parent_scope)
> + return parent_scope->break_is_for_switchcase();
> +
> + return false;
> +}
> +
> +int prog_scope::id() const
> +{
> + return scope_id;
> +}
> +
> +int prog_scope::begin() const
> +{
> + return scope_begin;
> +}
> +
> +int prog_scope::end() const
> +{
> + return scope_end;
> +}
> +
> +void prog_scope::set_previous_case_scope(prog_scope * prev)
> +{
> + previous_case_scope = prev;
> +}
> +
> +void prog_scope::set_end(int end)
> +{
> + if (scope_end == -1) {
> + scope_end = end;
> + if (previous_case_scope)
> + previous_case_scope->set_end(end);
> + }
> +}
> +
> +void prog_scope::set_loop_break_line(int line)
> +{
> + if (scope_type == loop_body) {
> + break_loop_line = line;
This should only be updated for the first break, right? I.e.,
break_loop_line = MIN2(break_loop_line, line);
> + } else {
> + if (parent_scope)
> + parent()->set_loop_break_line(line);
> + }
> +}
> +
> +int prog_scope::loop_break_line() const
> +{
> + return break_loop_line;
> +}
> +
> +temp_access::temp_access():
> + first_access_mask(0),
> + summary_access_mask(0),
> + needs_component_tracking(false)
> +{
> +}
> +
> +void temp_access::update_access_mask(int mask)
> +{
> + if (!first_access_mask) {
> + first_access_mask = mask;
> + } else {
> + needs_component_tracking |= (first_access_mask != mask);
> + }
> + summary_access_mask |= mask;
This can be simplified to:
if (mask != access_mask)
needs_component_tracking = true;
access_mask |= mask;
saving one member variable.
> +}
> +
> +void temp_access::record_write(int line, prog_scope *scope, int writemask)
> +{
> + update_access_mask(writemask);
> +
> + if (writemask & WRITEMASK_X)
> + comp[0].record_write(line, scope);
> + if (writemask & WRITEMASK_Y)
> + comp[1].record_write(line, scope);
> + if (writemask & WRITEMASK_Z)
> + comp[2].record_write(line, scope);
> + if (writemask & WRITEMASK_W)
> + comp[3].record_write(line, scope);
> +}
> +
> +void temp_access::record_read(int line, prog_scope *scope, int swizzle)
> +{
> + int readmask = 0;
> + for (int idx = 0; idx < 4; ++idx) {
> + int swz = GET_SWZ(swizzle, idx);
> + readmask |= (1 << swz) & 0xF;
> + }
> + update_access_mask(readmask);
> +
> + if (readmask & WRITEMASK_X)
> + comp[0].record_read(line, scope);
> + if (readmask & WRITEMASK_Y)
> + comp[1].record_read(line, scope);
> + if (readmask & WRITEMASK_Z)
> + comp[2].record_read(line, scope);
> + if (readmask & WRITEMASK_W)
> + comp[3].record_read(line, scope);
> +
> +
Remove extra lines of whitespace.
> +}
> +
> +inline static lifetime make_lifetime(int b, int e)
> +{
> + lifetime lt;
> + lt.begin = b;
> + lt.end = e;
> + return lt;
> +}
> +
> +lifetime temp_access::get_required_lifetime()
> +{
> + /* The register was never touched */
> + if (!summary_access_mask)
> + return make_lifetime(-1, -1);
> +
> + /* find first component that was actually used */
Capitalize comment.
> + int life_component = 0;
first_component?
> + while (!((1 << life_component) & summary_access_mask))
> + ++life_component;
> +
> + lifetime result = comp[life_component].get_required_lifetime();
Just use ffs(access_mask).
Actually, about the loop below: Is there an issue if there is a gap in
the used channels? E.g. channel X and Z are used, but Y never is?
One approach that could simplify the structure overall and probably fix
that a bug with gaps is this:
lifetime result;
result.begin = result.end = -1;
unsigned mask = access_mask;
while (mask) {
unsigned chan = u_bit_scan(&mask);
lifetime lt = comp[chan].get_required_lifetime();
... lifetime merging code here ...
if (!needs_component_tracking)
break;
}
return result;
This even avoids the initial check for the special case access_mask == 0.
> +
> + /* If the access mask changed, then merge per component life times */
> + if (needs_component_tracking) {
> +
> + RENAME_DEBUG(cerr << " (Needed component tracking) ");
> + for (int i = life_component+1; i < 4; ++i) {
Spaces around operator.
> + lifetime lt = comp[i].get_required_lifetime();
> +
> + if (lt.begin >= 0) {
> + if ( (result.begin < 0) || (result.begin > lt.begin))
Extra space between ( and (
> + result.begin = lt.begin;
> + }
> +
> + if (lt.end > result.end)
> + result.end = lt.end;
> + }
> + }
> +
> + return result;
> +}
> +
> +temp_comp_access::temp_comp_access():
> + last_read_scope(nullptr),
> + first_read_scope(nullptr),
> + first_dominant_write_scope(nullptr),
> + first_dominant_write(-1),
> + last_read(-1),
> + last_write(-1),
> + first_read(numeric_limits<int>::max())
> +{
> +}
> +
> +void temp_comp_access::record_read(int line, prog_scope *scope)
> +{
> + last_read_scope = scope;
> + last_read = line;
> +
> + if (first_read > line) {
> + first_read = line;
> + first_read_scope = scope;
> + }
> +}
> +
> +void temp_comp_access::record_write(int line, prog_scope *scope)
> +{
> + last_write = line;
> +
> + if (first_dominant_write < 0) {
> + first_dominant_write = line;
> + first_dominant_write_scope = scope;
> + }
Since this is no longer being updated on-the-fly, it doesn't have
anything to do with the concept of dominance in CFGs anymore. Please
just rename the variables to first_write / first_write_scope.
> +}
> +
> +void temp_comp_access::propagate_lifetime_to_dominant_write_scope()
> +{
> + first_dominant_write = first_dominant_write_scope->begin();
> + int lr = first_dominant_write_scope->end();
> +
> + if (last_read < lr)
> + last_read = lr;
> +}
> +
> +lifetime temp_comp_access::get_required_lifetime()
> +{
> + bool keep_for_full_loop = false;
> +
> + /* This register component is not used at all, mark it as unused,
> + * and ignore it in renaming.
> + */
> + if (last_write < 0 && last_read < 0)
> + return make_lifetime(-1, -1);
> +
> + /* this component is only read, this is undefined
Capitalize comment.
> + * behaviour, to distinguis it from unused components, we set
*distinguish
> + * the life time to [lr,lr), so it can me merged "away"
> + */
> + if (last_write < 0)
> + return make_lifetime(last_read, last_read);
Ah I see. I think it'd be better to return (-1, -1) here and handle that
case when merging the lifetimes. It's not more code, since it allows you
to merge the first two case in this function, and it avoids an overly
conservative extension of the register lifetime in the unlikely case
where a component is read from outside the scope of the other components.
> + assert(first_dominant_write_scope);
> +
> + /* Only written to, just make sure the register component is not
> + * reused in the range it is used to write to
> + */
> + if (!last_read_scope)
> + return make_lifetime(first_dominant_write, last_write + 1);
> +
> + const prog_scope *target_read_scope = first_read_scope;
> + const prog_scope *target_write_scope = first_dominant_write_scope;
What does "target read scope" mean? Similar for "target write scope"?
I'd be happier if we had more meaningful names for these, but it's not a
dealbreaker.
> +
> + /* We read before writing in a loop
> + * hence the value must survive the loops
> + */
> + if ((first_read <= first_dominant_write) &&
> + first_read_scope->is_in_loop()) {
> + keep_for_full_loop = true;
Extra space before =
> + target_read_scope = first_read_scope->outermost_loop();
> + }
> +
> + /* A conditional write within a nested loop must survive
> + * the outermost loop, but only if it is read outside
> + * the condition scope where we write.
> + */
> + const prog_scope *conditional = target_write_scope->enclosing_conditional();
> + if (conditional && conditional->is_in_loop() &&
> + !conditional->contains_range_of(*last_read_scope)) {
> + keep_for_full_loop = true;
Extra space before =
> + target_write_scope = conditional->outermost_loop();
> + }
> +
> + /* Evaluate the scope that is shared by all: required first write scope,
> + * required first read before write scope, and last read scope.
> + */
> + int enclosing_scope_depth = -1;
> + while (enclosing_scope_depth < 0) {
> +
Extra line of whitespace.
> + if (target_read_scope->contains_range_of(*target_write_scope) &&
> + target_read_scope->contains_range_of(*last_read_scope)) {
> + enclosing_scope_depth = target_read_scope->nesting_depth();
> + break;
> + }
> +
> + if (target_write_scope->contains_range_of(*target_read_scope) &&
> + target_write_scope->contains_range_of(*last_read_scope)) {
> + enclosing_scope_depth = target_write_scope->nesting_depth();
> + break;
> + }
> +
> + if (last_read_scope->contains_range_of(*target_read_scope) &&
> + last_read_scope->contains_range_of(*target_write_scope)) {
> + enclosing_scope_depth = last_read_scope->nesting_depth();
> + break;
> + }
> +
> + target_read_scope = target_read_scope->parent();
> + assert(target_read_scope);
> + }
This loop seems overly complicated, especially considering that you
repeatedly check target_write_scope and last_read_scope, but only really
modify target_read_scope.
How about something like this:
const prog_scope *enclosing_scope = last_read_scope;
if (target_write_scope->contains_range_of(*enclosing_scope))
enclosing_scope = target_write_scope;
if (target_read_scope->contains_range_of(*enclosing_scope))
enclosing_scope = target_read_scope;
while (!enclosing_scope->contains_range_of(*target_write_scope) ||
!enclosing_scope->contains_range_of(*target_read_scope))
enclosing_scope = enclosing_scope->parent();
That way, you can also drop the separate enclosing_scope_depth variable.
> +
> + /* propagate the last read scope to the target scope */
Capitalize comment.
> + while (last_read_scope->nesting_depth() > enclosing_scope_depth) {
> + /* if the read is in a loop we need to extend the
> + * variables life time to the end of that loop because at this
> + * point we know that it is not written in the same loop
> + */
Capitalize comment.
Also, I don't follow the comment. It's not true that we necessarily need
to extend the loop:
MOV TMP[0], ...
BGNLOOP
...
MOV TMP[0], ...
...
MOV ..., TMP[0]
...
ENDLOOP
So your code does an over-approximation, which is okay, but please
update the comment to reflect that.
> + if (last_read_scope->is_loop())
> + last_read = last_read_scope->end();
> +
> + last_read_scope = last_read_scope->parent();
> + }
> +
> + /* If the variable has to be kept for the whole loop, and we
> + * are currently in a loop, then propagate the life time.
> + */
> + if (keep_for_full_loop && first_dominant_write_scope->is_loop())
> + propagate_lifetime_to_dominant_write_scope();
> +
> + /* propagate the first_dominant_write scope to the target scope */
Capitalize comment.
> + while (enclosing_scope_depth < first_dominant_write_scope->nesting_depth()) { > +
Remove extra line of whitespace.
> + /* propagate lifetime if there was a break in a loop and the write was
Capitalize comment.
> + * after the break inside that loop. Note, that this is only needed if
> + * we move up in the scopes.
> + */
> + if (first_dominant_write_scope->loop_break_line() < first_dominant_write) {
> + keep_for_full_loop = true;
> + propagate_lifetime_to_dominant_write_scope();
> + }
> +
> + first_dominant_write_scope = first_dominant_write_scope->parent();
> +
> + /* Propagte lifetime if we are now in a loop */
> + if (keep_for_full_loop && first_dominant_write_scope->is_loop())
> + propagate_lifetime_to_dominant_write_scope();
> + }
> +
> + /* The last write past the last read is dead code, but since we
> + * talk component here we may still encounter it, and then
> + * we have to ensure that the component is not reused too
> + * early, hence extend the lifetime past the last write.
> + */
> + if (last_write >= last_read)
> + last_read = last_write + 1;
This is correct, but it's not really related to per-component tracking,
so the comment is a bit misleading.
> +
> + /* Here we are at the same scope, all is resolved */
> + return make_lifetime(first_dominant_write, last_read);
> +}
> +
> +/* Code below used for debugging */
> +#ifndef NDEBUG
> +static const char swizzle_txt[5] = "xyzw";
Drop the explicit array length, just say swizzle_txt[].
> +
> +static const char *file_names[PROGRAM_FILE_MAX] = {
> + "TEMP", "ARRAY", "IN", "OUT", "STATE", "CONST",
> + "UNIFORM", "WO", "ADDR", "SAMPLER", "SV", "UNDEF",
> + "IMM", "BUF", "MEM", "IMAGE"
> +};
Use _mesa_register_file_name instead.
> +
> +debug_log::debug_log()
> +{
> + enabled = std::getenv("GLSL_TO_TGSI_RENAME_DEBUG") != nullptr;
> +}
> +
> +bool debug_log::is_enabled()
> +{
> + static debug_log me;
> + return me.enabled;
> +}
> +
> +static
> +void dump_instruction(int line, prog_scope *scope,
> + const glsl_to_tgsi_instruction& inst)
> +{
> + const struct tgsi_opcode_info *info = tgsi_get_opcode_info(inst.op);
> +
> + int indent = scope->nesting_depth();
> + if ((scope->type() == switch_case_branch ||
> + scope->type() == switch_default_branch) &&
> + (info->opcode == TGSI_OPCODE_CASE ||
> + info->opcode == TGSI_OPCODE_DEFAULT))
> + --indent;
> +
> + if (info->opcode == TGSI_OPCODE_ENDIF ||
> + info->opcode == TGSI_OPCODE_ELSE ||
> + info->opcode == TGSI_OPCODE_ENDLOOP ||
> + info->opcode == TGSI_OPCODE_ENDSWITCH)
> + --indent;
> +
> + cerr << setw(4) << line << ": ";
Bad code alignment.
Cheers,
Nicolai
> + for (int i = 0; i < indent; ++i)
> + cerr << " ";
> + cerr << tgsi_get_opcode_name(info->opcode) << " ";
> +
> + bool has_operators = false;
> + for (unsigned j = 0; j < num_inst_dst_regs(&inst); j++) {
> + has_operators = true;
> + if (j > 0)
> + cerr << ", ";
> +
> + const st_dst_reg& dst = inst.dst[j];
> + cerr << file_names[dst.file]
> + << "[" << dst.index << "]";
> + if (dst.writemask != TGSI_WRITEMASK_XYZW) {
> + cerr << ".";
> + if (dst.writemask & TGSI_WRITEMASK_X) cerr << "x";
> + if (dst.writemask & TGSI_WRITEMASK_Y) cerr << "y";
> + if (dst.writemask & TGSI_WRITEMASK_Z) cerr << "z";
> + if (dst.writemask & TGSI_WRITEMASK_W) cerr << "w";
> + }
> + }
> + if (has_operators)
> + cerr << " := ";
> +
> + for (unsigned j = 0; j < num_inst_src_regs(&inst); j++) {
> + if (j > 0)
> + cerr << ", ";
> +
> + const st_src_reg& src = inst.src[j];
> + cerr << file_names[src.file]
> + << "[" << src.index << "]";
> + if (src.swizzle != SWIZZLE_XYZW) {
> + cerr << ".";
> + for (int idx = 0; idx < 4; ++idx) {
> + int swz = GET_SWZ(src.swizzle, idx);
> + if (swz < 4) {
> + cerr << swizzle_txt[swz];
> + }
> + }
> + }
> + }
> +
> + if (inst.tex_offset_num_offset > 0) {
> + cerr << ", TEXOFS: ";
> + for (unsigned j = 0; j < inst.tex_offset_num_offset; j++) {
> + if (j > 0)
> + cerr << ", ";
> +
> + const st_src_reg& src = inst.tex_offsets[j];
> + cerr << file_names[src.file]
> + << "[" << src.index << "]";
> + if (src.swizzle != SWIZZLE_XYZW) {
> + cerr << ".";
> + for (int idx = 0; idx < 4; ++idx) {
> + int swz = GET_SWZ(src.swizzle, idx);
> + if (swz < 4) {
> + cerr << swizzle_txt[swz];
> + }
> + }
> + }
> + }
> + }
> + cerr << "\n";
> +}
> +#endif
> \ No newline at end of file
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi_temprename.h b/src/mesa/state_tracker/st_glsl_to_tgsi_temprename.h
> new file mode 100644
> index 0000000000..44998cca97
> --- /dev/null
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi_temprename.h
> @@ -0,0 +1,55 @@
> +/*
> + * Copyright © 2017 Gert Wollny
> + *
> + * 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.
> + */
> +
> +#ifndef MESA_GLSL_TO_TGSI_TEMPRENAME_H
> +#define MESA_GLSL_TO_TGSI_TEMPRENAME_H
> +
> +#include "st_glsl_to_tgsi_private.h"
> +
> +/** Storage to record the required life time of a temporary register
> + * begin == end == -1 indicates that the register can be reused without
> + * limitations. Otherwise, "begin" indicates the first instruction in which
> + * a write operation may target this temporary, and end indicates the
> + * last instruction in which a value can be read from this temporary.
> + * Hence, a register R2 can be merged with a register R1 if R1.end <= R2.begin.
> + */
> +struct lifetime {
> + int begin;
> + int end;
> +};
> +
> +/** Evaluates the required life times of temporary registers in a shader
> + * @param[in] mem_ctx a memory context that can be used with the ralloc_* functions
> + * @param[in] instructions the shader to be anlzyed
> + * @param[in] ntemps number of temporaries reserved for this shader
> + * @param[in,out] lifetimes memory location to store the estimated required
> + * life times for each temporary register. The parameter must point to
> + * allocated memory that can hold ntemps lifetime structures. On output
> + * the life times contains the life times for the registers with the
> + * exception of TEMP[0].
> + */
> +void
> +get_temp_registers_required_lifetimes(void *mem_ctx, exec_list *instructions,
> + int ntemps, struct lifetime *lifetimes);
> +
> +#endif
> \ No newline at end of file
>
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
More information about the mesa-dev
mailing list