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.
>> > 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?
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
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
