Re: [Mesa-dev] [PATCH v6 2/6] mesa/st: glsl_to_tgsi: implement new temporary register lifetime tracker
On 17.07.2017 04:32, Gert Wollny wrote:
Just a few comments while updating the patch:
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.
Actually, debug_log is implemented as a singleton, and the constructor
that calles getenv is only called once, but yes, there is no need to
make it a class.
Ah yes, you're right, missed the static. Still would be clearer as not a
class, I think.
+
+ 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.
Since SWITCH currently is not really generated it is difficult to test
what byte code would actually be later generated, but I figured that in
the worst case the SWITCH would translate to an IF-chain, in which case
for each CASE statement one would actually need to read both, the
SWITCH value and the CASE value.
The only thing that actually implements TGSI_OPCODE_SWITCH is
tgsi_exec.c, and it only reads the switch value when it encounters the
SWITCH statement.
So I think you're right, it's good to have the reads on the CASE source
on the line of the CASE, but I also think the read of the SWITCH source
should only be one the SWITCH line. It simplifies the code here, and
matches the only possible user.
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
I don't think it is necessary, and I will check whether it breaks
something.
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?
I think that this case is handled consistently.
Anyway, I will contemplate about whether it makes sense to keep this
behavior, and I actually lean towards removing it.
Thanks.
+
+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.
Actually, it must be
if (access_mask && (mask != access_mask))
needs_component_tracking = true;
...
otherwise needs_component_tracking would always be set to true.
Right, good point.
+* 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.
My thinking was to completely ignore unused registers, and use read-
only registers for renaming, but now I checked that a register that is
only read is actually eliminated in
glsl_to_tgsi_visitor::renumber_registers
so no need to bother distinguishing the two cases.
Great!
Cheers,
Nicolai
Best,
Gert
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v6 2/6] mesa/st: glsl_to_tgsi: implement new temporary register lifetime tracker
Just a few comments while updating the patch:
> 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.
Actually, debug_log is implemented as a singleton, and the constructor
that calles getenv is only called once, but yes, there is no need to
make it a class.
>
> > +
> > + 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.
Since SWITCH currently is not really generated it is difficult to test
what byte code would actually be later generated, but I figured that in
the worst case the SWITCH would translate to an IF-chain, in which case
for each CASE statement one would actually need to read both, the
SWITCH value and the CASE value.
> 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
I don't think it is necessary, and I will check whether it breaks
something.
> 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?
I think that this case is handled consistently.
Anyway, I will contemplate about whether it makes sense to keep this
behavior, and I actually lean towards removing it.
> +
> > +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.
Actually, it must be
if (access_mask && (mask != access_mask))
needs_component_tracking = true;
...
otherwise needs_component_tracking would always be set to true.
>
> > +* 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.
My thinking was to completely ignore unused registers, and use read-
only registers for renaming, but now I checked that a register that is
only read is actually eliminated in
glsl_to_tgsi_visitor::renumber_registers
so no need to bother distinguishing the two cases.
Best,
Gert
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v6 2/6] mesa/st: glsl_to_tgsi: implement new temporary register lifetime tracker
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 00..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
+#include
+#include
The mesa/ should be unnecessary and dropped for consistency.
+#include
+#include
+
+/* std::sort is significantly faster than qsort */
+#define USE_STL_SORT
+#ifdef USE_STL_SORT
+#include
+#endif
+
+#ifndef NDEBUG
+#include
+#include
+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
