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
