On Thu, 2015-09-03 at 13:15 +0300, Francisco Jerez wrote:
> Iago Toral <ito...@igalia.com> writes:
>
> > On Wed, 2015-09-02 at 17:53 +0300, Francisco Jerez wrote:
> >> Iago Toral <ito...@igalia.com> writes:
> >>
> >> > On Wed, 2015-09-02 at 14:29 +0300, Francisco Jerez wrote:
> >> >> Iago Toral <ito...@igalia.com> writes:
> >> >>
> >> >> > Hi Curro,
> >> >> >
> >> >> > I have been a couple of weeks on holidays and have just come back to
> >> >> > this:
> >> >> >
> >> >> > On Thu, 2015-08-06 at 18:27 +0300, Francisco Jerez wrote:
> >> >> >> Iago Toral Quiroga <ito...@igalia.com> writes:
> >> >> >>
> >> >> >> > If we have spilled/unspilled a register in the current
> >> >> >> > instruction, avoid
> >> >> >> > emitting unspills for the same register in the same instruction or
> >> >> >> > consecutive
> >> >> >> > instructions following the current one as long as they keep
> >> >> >> > reading the spilled
> >> >> >> > register. This should allow us to avoid emitting costy unspills
> >> >> >> > that come with
> >> >> >> > little benefit to register allocation.
> >> >> >> >
> >> >> >> > Also, update evaluate_spill_costs so that we account for the saved
> >> >> >> > unspills.
> >> >> >> > ---
> >> >> >> > .../drivers/dri/i965/brw_vec4_reg_allocate.cpp | 129
> >> >> >> > +++++++++++++++++++--
> >> >> >> > 1 file changed, 121 insertions(+), 8 deletions(-)
> >> >> >> >
> >> >> >> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_reg_allocate.cpp
> >> >> >> > b/src/mesa/drivers/dri/i965/brw_vec4_reg_allocate.cpp
> >> >> >> > index 617c988..fed5f4d 100644
> >> >> >> > --- a/src/mesa/drivers/dri/i965/brw_vec4_reg_allocate.cpp
> >> >> >> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_reg_allocate.cpp
> >> >> >> > @@ -264,6 +264,95 @@ vec4_visitor::reg_allocate()
> >> >> >> > return true;
> >> >> >> > }
> >> >> >> >
> >> >> >> > +/**
> >> >> >> > + * When we decide to spill a register, instead of blindly
> >> >> >> > spilling every use,
> >> >> >> > + * save unspills when the spill register is used (read) in
> >> >> >> > consecutive
> >> >> >> > + * instructions. This can potentially save a bunch of unspills
> >> >> >> > that would
> >> >> >> > + * have very little impact in register allocation anyway.
> >> >> >> > + *
> >> >> >> > + * Notice that we need to account for this behavior when spilling
> >> >> >> > a register
> >> >> >> > + * and when evaluating spilling costs. This function is designed
> >> >> >> > so it can
> >> >> >> > + * be called from both places and avoid repeating the logic.
> >> >> >> > + *
> >> >> >> > + * - When we call this function from spill_reg, we pass in
> >> >> >> > scratch_reg the
> >> >> >> > + * actual unspill/spill register that we want to reuse in the
> >> >> >> > current
> >> >> >> > + * instruction.
> >> >> >> > + *
> >> >> >> > + * - When we call this from evaluate_spill_costs, we pass the
> >> >> >> > register for
> >> >> >> > + * which we are evaluating spilling costs.
> >> >> >> > + *
> >> >> >> > + * In either case, we check if the previous instructions read
> >> >> >> > scratch_reg until
> >> >> >> > + * we find an instruction that writes to it (in which case we can
> >> >> >> > reuse
> >> >> >> > + * scratch_reg as long as the writemask is compatible with the
> >> >> >> > channels we need
> >> >> >> > + * to read in the current instruction) or we hit an instruction
> >> >> >> > that does not
> >> >> >> > + * read scratch_reg at all. The latter can only happen when we
> >> >> >> > call this from
> >> >> >> > + * evaluate_spill_costs,
> >> >> >>
> >> >> >> Strictly speaking it can also happen when called from spill_reg() for
> >> >> >> the first time in a given sequence of consecutive instructions (in
> >> >> >> which
> >> >> >> case you correctly return false).
> >> >> >
> >> >> > not really, spill_reg() knows if it is the first time that it is
> >> >> > spilling a register and won't call this function in that case.
> >> >> >
> >> >> You may have several disjoint sequences of consecutive instructions
> >> >> using spill_reg_nr repeatedly. The check you have in spill_reg() will
> >> >> only help you for the first one sequence, so it's in fact redundant
> >> >> because can_use_scratch_for_source() seems to handle the case in which
> >> >> the register access is the first in a sequence just fine anyway.
> >> >
> >> > Ah, right. I'll update the comment accordingly.
> >> >
> >> >> >> > and means that this is the point at which we first
> >> >> >> > + * need the unspill this register for our current instruction.
> >> >> >> > Since all our
> >> >> >> > + * unspills read a full vec4, we know that in this case we will
> >> >> >> > have all
> >> >> >> > + * the channels available in scratch_reg and we can reuse it.
> >> >> >> > + *
> >> >> >> > + * In any other case, we can't reuse scratch_reg in the current
> >> >> >> > instruction,
> >> >> >> > + * meaning that we will need to unspill it.
> >> >> >> > + */
> >> >> >> > +static bool
> >> >> >> > +can_use_scratch_for_source(const vec4_instruction *inst, unsigned
> >> >> >> > i,
> >> >> >> > + unsigned scratch_reg)
> >> >> >> > +{
> >> >> >> > + assert(inst->src[i].file == GRF);
> >> >> >> > +
> >> >> >> > + /* If the current instruction is already using scratch_reg in
> >> >> >> > src[n] with
> >> >> >> > + * n < i, then we know we can reuse it for src[i] too.
> >> >> >> > + */
> >> >> >> > + for (unsigned n = 0; n < i; n++) {
> >> >> >> > + if (inst->src[n].file == GRF && inst->src[n].reg ==
> >> >> >> > scratch_reg)
> >> >> >> > + return true;
> >> >> >> > + }
> >> >> >>
> >> >> >> I don't think this is correct in cases where the previous source
> >> >> >> reused
> >> >> >> the temporary of a previously spilled register with incompatible
> >> >> >> writemask. You probably want to handle the current instruction
> >> >> >> consistently with the previous ones, i.e. as part of the loop below.
> >> >> >>
> >> >> >> I suggest you define a variable (e.g. n as you've called it)
> >> >> >> initially
> >> >> >> equal to i that would determine the number of sources to check for
> >> >> >> the
> >> >> >> next instruction. At the end of the loop body it would be re-set to
> >> >> >> 3,
> >> >> >> what would also cause the destination registers to be checked in
> >> >> >> subsequent iterations.
> >> >> >
> >> >> > I have been thinking a bit about this and decided that it was simpler
> >> >> > to
> >> >> > keep this part as it is in this patch and simply fix the condition to
> >> >> > check that the swizzle we read in src[i] is a subset of the swizzle in
> >> >> > src[n] too. I think that using the value of 'n' like you suggest would
> >> >> > make things a bit less clear in the loop below since we would be using
> >> >> > the value of n to tell if we are processing the current instruction
> >> >> > or a
> >> >> > previous instruction (and thus decide if we need to check the dst or
> >> >> > not), which is not an obvious association. In any case, let me know if
> >> >> > you have a strong preference for your implementation and I'll change
> >> >> > that.
> >> >> >
> >> >> Wouldn't that be unnecessarily pessimistic if the n-th argument happens
> >> >> to read a subset of the components which are available in scratch_reg?
> >> >
> >> > I think it wouldn't. That loop only checks if we can make the decision
> >> > of reusing scratch_reg early only by looking at previous srcs in the
> >> > current instruction. If none of the previous srcs in the same
> >> > instruction reads scratch_reg with a compatible readmask we do not
> >> > return FALSE, we just we just do as usual and check previous
> >> > instructions, and we make the decision based on that.
> >> >
> >>
> >> You won't directly return false, but prev_inst_read_scratch_reg is left
> >> equal to false and if there's no previous read or write of the same
> >> register you'll return false incorrectly.
> >
> > If a previous src in the same instruction uses scratch_reg it means that
> > we have already checked that previous instructions read/write to
> > scratch_reg (that is why we decided to use it for that other src in the
> > same instruction), so we should never hit the case you mention, right?
> >
> Unless there's no previous read or write, say, if you're called from
> evaluate_spill_costs().
Yes, you're right. I'll implement the suggestion you made above. Thanks
for the patience ;)
Iago
> >> Anyway I was suggesting that
> >> because it's also likely to be easier than having to check the swizzles
> >> explicitly.
> >
> > That is only a matter of doing something like:
> >
> > unsigned swizzle_mask_i = brw_mask_for_swizzle(inst->src[i].swizzle);
> > unsigned swizzle_mask_n = brw_mask_for_swizzle(inst->src[n].swizzle);
> >
> > And then add this to the condition:
> >
> > (swizzle_mask_i & swizzle_mask_n) == swizzle_mask_i
> >
> > In any case, I have no issues with doing what you suggest, it is not
> > like this is a relevant optimization anyway, I just want to understand
> > whether the implementation could really have any issues that I am still
> > missing.
> >
> >> >> If you want to keep the loop unrolled, I suggest you simply move the
> >> >> prev_inst_read_scratch_reg declaration up and have the first loop set it
> >> >> to true if any of the sources reads from scratch_reg instead of exiting
> >> >> the function.
> >> >> >> > +
> >> >> >> > + bool prev_inst_read_scratch_reg = false;
> >> >> >> > + vec4_instruction *prev_inst = (vec4_instruction *) inst->prev;
> >> >> >>
> >> >> >> You can move this declaration into the init statement of the for
> >> >> >> loop to
> >> >> >> limit its scope.
> >> >> >>
> >> >> >> > + for (; !prev_inst->is_head_sentinel();
> >> >> >> > + prev_inst = (vec4_instruction *) prev_inst->prev) {
> >> >> >> > + /* If any previous instruction does not read from or write
> >> >> >> > to scratch_reg
> >> >> >> > + * inconditonally we cannot reuse scratch_reg
> >> >> >> > + */
> >> >> >> > + if (prev_inst->predicate && prev_inst->opcode !=
> >> >> >> > BRW_OPCODE_SEL)
> >> >> >> > + return false;
> >> >> >>
> >> >> >> I think this is somewhat pessimistic, register fills for a predicated
> >> >> >> instruction won't be predicated AFAIK, so it should be possible to
> >> >> >> reuse
> >> >> >> them, only the destination of a predicated write cannot be reused.
> >> >> >>
> >> >> >> > +
> >> >> >> > + /* If the previous instruction writes to scratch_reg then
> >> >> >> > we can reuse
> >> >> >> > + * it if the channels we wrote are compatible with our read
> >> >> >> > mask.
> >> >> >> > + */
> >> >> >> > + if (prev_inst->dst.file == GRF && prev_inst->dst.reg ==
> >> >> >> > scratch_reg) {
> >> >> >> > + return (brw_mask_for_swizzle(inst->src[i].swizzle) &
> >> >> >> > + ~prev_inst->dst.writemask) == 0;
> >> >> >> > + }
> >> >> >> > +
> >> >> >> > + if (prev_inst->opcode == SHADER_OPCODE_GEN4_SCRATCH_WRITE ||
> >> >> >> > + prev_inst->opcode == SHADER_OPCODE_GEN4_SCRATCH_READ)
> >> >> >> > + continue;
> >> >> >> > +
> >> >> >> I'm not sure I see why you need to treat scratch reads and writes
> >> >> >> specially here. AFAICT if you come across one for the same
> >> >> >> scratch_reg
> >> >> >> it won't make a difference for the code below, and if you come across
> >> >> >> one for a different register the condition you wanted to check (the
> >> >> >> same
> >> >> >> register is reused for a number of consecutive instructions) may no
> >> >> >> longer hold, so you may as well return.
> >> >> >>
> >> >> >> > + /* Check that the previous instruction reads scratch_reg,
> >> >> >> > if so, continue
> >> >> >> > + * looping. Otherwise it means that we got here from
> >> >> >> > + * evaluate_spill_costs and this is the point at which we
> >> >> >> > will emit an
> >> >> >> > + * unspill for the register passed in scratch_reg, in which
> >> >> >> > case we can
> >> >> >> > + * only reuse it if all other instructions in between have
> >> >> >> > read
> >> >> >> > + * scratch_reg.
> >> >> >> > + */
> >> >> >> > + int n;
> >> >> >> > + for (n = 0; n < 3; n++) {
> >> >> >> > + if (prev_inst->src[n].file == GRF &&
> >> >> >> > + prev_inst->src[n].reg == scratch_reg) {
> >> >> >> > + prev_inst_read_scratch_reg = true;
> >> >> >> > + break;
> >> >> >> > + }
> >> >> >> > + }
> >> >> >> > + if (n == 3) {
> >> >> >> > + return prev_inst_read_scratch_reg;
> >> >> >> > + }
> >> >> >> > + }
> >> >> >> > +
> >> >> >> > + return false;
> >> >> >>
> >> >> >> Shouldn't this return prev_inst_read_scratch_reg?
> >> >> >>
> >> >> >> > +}
> >> >> >> > +
> >> >> >> > void
> >> >> >> > vec4_visitor::evaluate_spill_costs(float *spill_costs, bool
> >> >> >> > *no_spill)
> >> >> >> > {
> >> >> >> > @@ -281,9 +370,15 @@ vec4_visitor::evaluate_spill_costs(float
> >> >> >> > *spill_costs, bool *no_spill)
> >> >> >> > foreach_block_and_inst(block, vec4_instruction, inst, cfg) {
> >> >> >> > for (unsigned int i = 0; i < 3; i++) {
> >> >> >> > if (inst->src[i].file == GRF) {
> >> >> >> > - spill_costs[inst->src[i].reg] += loop_scale;
> >> >> >> > - if (inst->src[i].reladdr)
> >> >> >> > - no_spill[inst->src[i].reg] = true;
> >> >> >> > + /* We will only unspill src[i] it it wasn't unspilled
> >> >> >> > for the
> >> >> >> > + * previous instruction, in which case we'll just
> >> >> >> > reuse the scratch
> >> >> >> > + * reg for this instruction.
> >> >> >> > + */
> >> >> >> > + if (!can_use_scratch_for_source(inst, i,
> >> >> >> > inst->src[i].reg)) {
> >> >> >> > + spill_costs[inst->src[i].reg] += loop_scale;
> >> >> >> > + if (inst->src[i].reladdr)
> >> >> >> > + no_spill[inst->src[i].reg] = true;
> >> >> >> > + }
> >> >> >> > }
> >> >> >> > }
> >> >> >> >
> >> >> >> > @@ -342,19 +437,37 @@ vec4_visitor::spill_reg(int spill_reg_nr)
> >> >> >> > unsigned int spill_offset = last_scratch++;
> >> >> >> >
> >> >> >> > /* Generate spill/unspill instructions for the objects being
> >> >> >> > spilled. */
> >> >> >> > + int scratch_reg = -1;
> >> >> >> > foreach_block_and_inst(block, vec4_instruction, inst, cfg) {
> >> >> >> > + /* We don't spill registers used for scratch */
> >> >> >> > + if (inst->opcode == SHADER_OPCODE_GEN4_SCRATCH_READ ||
> >> >> >> > + inst->opcode == SHADER_OPCODE_GEN4_SCRATCH_WRITE)
> >> >> >> > + continue;
> >> >> >> > +
> >> >> >> For that reason you'll never see a match in the loop below for
> >> >> >> scratch
> >> >> >> reads and writes, and it should be a harmless no-op.
> >> >> >>
> >> >> >> > for (unsigned int i = 0; i < 3; i++) {
> >> >> >> > if (inst->src[i].file == GRF && inst->src[i].reg ==
> >> >> >> > spill_reg_nr) {
> >> >> >> > - src_reg spill_reg = inst->src[i];
> >> >> >> > - inst->src[i].reg = alloc.allocate(1);
> >> >> >> > - dst_reg temp = dst_reg(inst->src[i]);
> >> >> >> > -
> >> >> >> > - emit_scratch_read(block, inst, temp, spill_reg,
> >> >> >> > spill_offset);
> >> >> >> > + if (scratch_reg == -1 ||
> >> >> >> > + !can_use_scratch_for_source(inst, i,
> >> >> >> > scratch_reg)) {
> >> >> >> > + /* We need to unspill anyway so make sure we read
> >> >> >> > the full vec4
> >> >> >> > + * in any case. This way, the cached register can
> >> >> >> > be reused
> >> >> >> > + * for consecutive instructions that read
> >> >> >> > different channels of
> >> >> >> > + * the same vec4.
> >> >> >> > + */
> >> >> >> > + scratch_reg = alloc.allocate(1);
> >> >> >> > + src_reg temp = inst->src[i];
> >> >> >> > + temp.reg = scratch_reg;
> >> >> >> > + temp.swizzle = BRW_SWIZZLE_XYZW;
> >> >> >> > + emit_scratch_read(block, inst,
> >> >> >> > + dst_reg(temp), inst->src[i],
> >> >> >> > spill_offset);
> >> >> >> > + }
> >> >> >> > + assert(scratch_reg != -1);
> >> >> >> > + inst->src[i].reg = scratch_reg;
> >> >> >> > }
> >> >> >> > }
> >> >> >> >
> >> >> >> > if (inst->dst.file == GRF && inst->dst.reg == spill_reg_nr)
> >> >> >> > {
> >> >> >> > emit_scratch_write(block, inst, spill_offset);
> >> >> >> > + scratch_reg = inst->dst.reg;
> >> >> >> > }
> >> >> >> > }
> >> >> >> >
> >> >> >> > --
> >> >> >> > 1.9.1
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
