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