On Fri, 2015-08-07 at 14:14 +0300, Francisco Jerez wrote:
> Iago Toral <ito...@igalia.com> writes:
>
> > 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).
> >
> > True, I'll fix the comment.
> >
> >> > 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.
> >
> > Yes, you're right.
> >
> >> 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.
> >
> > Sounds good to me.
> >
> >> > +
> >> > + 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.
> >
> > Ok.
> >
> >> > + 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.
> >
> > Yeah, makes sense.
> >
> >> > +
> >> > + /* 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.
> >
> > I had my doubts about that, but the reason was the following. Say we
> > have this code:
> >
> > r1 = r2 + r3
> > r4 = r2 * r3
> >
> > In theory, if r2 and r3 were to be spilled we would want to reuse the
> > scratch registers in the second instruction, but the truth is that if we
> > spill them we end up with code like this:
> >
> > r2 = scratch_load(offset_r2)
> > r3 = scratch_load(offset_r3)
> > r1 = r2 + r3
> > r4 = r2 + r3
> >
> > When we call this function for the instruction that writes r1, it will
> > see that the previous instruction uses r3, but _not_ r2, and decide that
> > it can't use the scratch result for r2 triggering a new load.
>
> Hm, I'm not following. Say we first decide to spill r2, the result will
> be:
>
> | r2 = scratch_load(offset_r2)
> | r1 = r2 + r3
> | r4 = r2 + r3
>
> If we then decide to spill r3 your code would notice that it's read from
> in two consecutive instructions and then go on and reuse it regardless
> of whether scratch reads and writes are treated specially or not, like:
>
> | r2 = scratch_load(offset_r2)
> | r3 = scratch_load(offset_r3)
> | r1 = r2 + r3
> | r4 = r2 + r3
>
> > This happens because each scratch load only pulls one register, so as
> > soon as we inject these in the code we are effectively limiting the
> > optimization to just one register.
> >
> > It can get worse. If we spill r1 before these, we would have:
> >
> > r2 = scratch_load(offset_r2)
> > r3 = scratch_load(offset_r3)
> > r1 = r2 + r3
> > write(r1, offset_r1)
> > r4 = r2 + r3
> >
>
> Ah, this seems like a better example. In this case r2 and/or r3 will be
> reused or not depending on the order in which the spills are done, what
> indeed is slightly disconcerting -- but note that in all such cases the
> original assumption you started from will be violated (the temporary is
> re-used if it was already read or written in a sequence of consecutive
> instructions). That said I don't really have any objection against
> special-casing scratch reads and writes, I just wanted to understand why
> you had decided to break your own rule. ;)
Yeah, I also thought that in this case it is ok-ish to break that
rule... however, there is one more concern I had with this. We can also
have scratch reads/writes because we lowered indirect GRF array accesses
to scratch... If we have that then, I may be abusing the rule a bit too
much here, but we can't tell which scratch operations are spills and
which are lowered GRF array accesses :-/
> > Now the instruction that writes to r4 can't reuse neither r2 nor r3
> > because the scratch write in between won't read any of them.
> >
> >> > + /* 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?
> >
> > Yes.
> >
> >> > +}
> >> > +
> >> > 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.
> >
> > I figured we would rather skip loop for the 3 srcs and the check for the
> > dst in this case since we already know that we won't see a match. I can
> > remove the check if you think it is not worth it though.
> >
> >
> >> > 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
