[Mesa-dev] [PATCH 07/10] i965/fs: Add pass to combine immediates.
Matt Turner
mattst88 at gmail.com
Tue Feb 17 16:17:48 PST 2015
On Tue, Feb 17, 2015 at 3:45 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Wednesday, February 04, 2015 08:21:24 PM Matt Turner wrote:
>> total instructions in shared programs: 5885407 -> 5940958 (0.94%)
>> instructions in affected programs: 3617311 -> 3672862 (1.54%)
>> helped: 3
>> HURT: 23556
>> GAINED: 31
>> LOST: 165
>>
>> ... but will allow us to always emit MAD instructions.
>> ---
>> src/mesa/drivers/dri/i965/Makefile.sources | 1 +
>> src/mesa/drivers/dri/i965/brw_fs.cpp | 2 +
>> src/mesa/drivers/dri/i965/brw_fs.h | 1 +
>> .../drivers/dri/i965/brw_fs_combine_constants.cpp | 254 +++++++++++++++++++++
>> 4 files changed, 258 insertions(+)
>> create mode 100644 src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp
>>
>> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources b/src/mesa/drivers/dri/i965/Makefile.sources
>> index 37697f3..c69441b 100644
>> --- a/src/mesa/drivers/dri/i965/Makefile.sources
>> +++ b/src/mesa/drivers/dri/i965/Makefile.sources
>> @@ -40,6 +40,7 @@ i965_FILES = \
>> brw_ff_gs.h \
>> brw_fs_channel_expressions.cpp \
>> brw_fs_cmod_propagation.cpp \
>> + brw_fs_combine_constants.cpp \
>> brw_fs_copy_propagation.cpp \
>> brw_fs.cpp \
>> brw_fs_cse.cpp \
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 3142ab4..69602a7 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -3612,6 +3612,8 @@ fs_visitor::optimize()
>> OPT(dead_code_eliminate);
>> }
>>
>> + opt_combine_constants();
>
> We should make it return a boolean so that we can do:
>
> OPT(opt_combine_constants);
>
> that way, INTEL_DEBUG=optimizer will print out the transformed IR
> correctly.
>
>> +
>> lower_uniform_pull_constant_loads();
>> }
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
>> index b95e2c0..c160bd6 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.h
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
>> @@ -471,6 +471,7 @@ public:
>> void no16(const char *msg, ...);
>> void lower_uniform_pull_constant_loads();
>> bool lower_load_payload();
>> + void opt_combine_constants();
>>
>> void emit_dummy_fs();
>> void emit_repclear_shader();
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp b/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp
>> new file mode 100644
>> index 0000000..ad4a965
>> --- /dev/null
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp
>> @@ -0,0 +1,254 @@
>> +/*
>> + * Copyright © 2014 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + */
>> +
>> +/** @file brw_fs_combine_constants.cpp
>> + *
>> + * This file contains the opt_combine_constants() pass that runs after the
>> + * regular optimization loop. It passes over the instruction list and
>> + * selectively promotes immediate values to registers by emitting a mov(1)
>> + * instruction.
>> + *
>> + * This is useful on Gen 7 particularly, because a few instructions can be
>> + * coissued (i.e., issued in the same cycle as another thread on the same EU
>> + * issues an instruction) under some circumstances, one of which is that they
>> + * cannot use immediate values.
>> + */
>> +
>> +#include "brw_fs.h"
>> +#include "brw_fs_live_variables.h"
>> +#include "brw_cfg.h"
>> +
>
> /**
> * Return whether an instruction woudl be coissuable if we replaced its
> * immediate source with a GRF.
> */
>
>> +static bool
>> +could_coissue(const fs_inst *inst)
>> +{
>> + /* MAD can coissue, but while the PRM lists various restrictions for Align1
>> + * instructions related to data alignment and regioning, it doesn't list
>> + * similar restrictions for Align16 instructions. I don't expect that there
>> + * are any restrictions, since Align16 doesn't allow the kinds of operations
>> + * that are restricted in Align1 mode.
>> + *
>> + * Since MAD is an Align16 instruction we assume it can always coissue.
>> + */
>
> This comment should go away, given that MAD isn't handled here.
>
> You mentioned we should probably also add:
>
> if (brw->gen < 7)
> return false;
>
> which seems like a good plan.
>
>> + switch (inst->opcode) {
>> + case BRW_OPCODE_MOV:
>> + case BRW_OPCODE_CMP:
>> + case BRW_OPCODE_ADD:
>> + case BRW_OPCODE_MUL:
>> + return true;
>> + default:
>> + return false;
>> + }
>> +}
>> +
>
> /** A box for putting fs_regs in a linked list. */
>
>> +struct reg_link {
>> + DECLARE_RALLOC_CXX_OPERATORS(reg_link)
>> +
>> + reg_link(fs_reg *reg) : reg(reg) {}
>> +
>> + struct exec_node link;
>> + fs_reg *reg;
>> +};
>> +
>> +static struct exec_node *
>> +link(void *mem_ctx, fs_reg *reg)
>> +{
>> + reg_link *l = new(mem_ctx) reg_link(reg);
>> + return &l->link;
>> +}
>> +
>
> /**
> * Information about an immediate value.
> *
> * For example, this would refer
> */
>
>> +struct imm {
>
> /** The common ancestor of all blocks using this immediate value. */
>
>> + bblock_t *block;
>
> /**
> * The instruction generating the immediate value, if all uses are
> * contained within a single basic block. Otherwise, NULL.
> */
>
>> + fs_inst *inst;
>
> /**
> * A list of fs_regs that refer to this immediate. If we promote it,
> * we'll have to patch these up to refer to the new GRF.
> */
>> + exec_list *uses;
>
> /** The immediate value. We currently only handle floats. */
>
>> + float val;
>> +
>
> /**
> * The GRF register and subregister number where we've decided to
> * store the constant value.
> */
>> + uint8_t subreg_offset;
>> + uint16_t reg;
>> +
>
> /** The number of coissuable instructions using this immediate. */
>
>> + uint16_t uses_by_coissue;
>> +
>> + bool inserted;
>
> The "inserted" field appears dead - you initialize it to false, and then
> check whether it's true later (but it never is). Perhaps remove it?
Ah, yes. It's used in some additional patches to emit VF immediates to
let us know not to reemit a value that was part of a VF.
I'll move it to that patch.
>
>> +
>> + uint16_t first_use_ip;
>> + uint16_t last_use_ip;
>> +};
>> +
>
> /** The working set of information about immediates. */
>
>> +struct table {
>> + struct imm *imm;
>> + int size;
>> + int len;
>> +};
>> +
>> +static struct imm *
>> +find_imm(struct table *table, float val)
>> +{
>> + assert(signbit(val) == 0);
>> +
>> + for (int i = 0; i < table->len; i++) {
>> + if (table->imm[i].val == val) {
>> + return &table->imm[i];
>> + }
>> + }
>> + return NULL;
>> +}
>> +
>> +static struct imm *
>> +new_imm(struct table *table, void *mem_ctx)
>> +{
>> + if (table->len == table->size) {
>> + table->size *= 2;
>> + table->imm = reralloc(mem_ctx, table->imm, struct imm, table->size);
>> + }
>> + return &table->imm[table->len++];
>> +}
>> +
>
> /**
> * Comparitor used for sorting an array of imm structures.
> *
> * We sort by basic block number, then last use IP, then first use IP
> * (least to greatest).
> *
> * XXX: perhaps explain why
Sure:
/**
* Comparator used for sorting an array of imm structures.
*
* We sort by basic block number, then last use IP, then first use IP (least
* to greatest). This sorting causes immediates live in the same area to be
* allocated to the same register in the hopes that all values will be dead
* about the same time and the register can be reused.
*/
> */
>> +static int
>> +compare(const void *_a, const void *_b)
>> +{
>> + const struct imm *a = (const struct imm *)_a,
>> + *b = (const struct imm *)_b;
>> +
>> + int block_diff = a->block->num - b->block->num;
>> + if (block_diff)
>> + return block_diff;
>> +
>> + int end_diff = a->last_use_ip - b->last_use_ip;
>> + if (end_diff)
>> + return end_diff;
>> +
>> + return a->first_use_ip - b->first_use_ip;
>> +}
>> +
>> +void
>> +fs_visitor::opt_combine_constants()
>> +{
>> + void *const_ctx = ralloc_context(NULL);
>> +
>> + struct table table;
>> + table.size = 8;
>> + table.len = 0;
>> + table.imm = ralloc_array(const_ctx, struct imm, table.size);
>> +
>> + cfg->calculate_idom();
>> + unsigned ip = -1;
>> +
>> + /* Make a pass through all instructions and count the number of times each
>> + * constant is used by coissueable instructions.
>> + */
>> + foreach_block_and_inst(block, fs_inst, inst, cfg) {
>> + ip++;
>> +
>> + if (!could_coissue(inst))
>> + continue;
>> +
>> + for (int i = 0; i < inst->sources; i++) {
>> + if (inst->src[i].file != IMM)
>> + continue;
>
> Given that you're calling fabsf on the value, and storing floats, I
> don't think this will work out at all for integers. I'm guessing it
> won't work for VFs either.
>
> We should probably do:
>
> if (inst->src[i].file != IMM ||
> inst->src[i].type != BRW_REGISTER_TYPE_F)
> continue;
>
>> +
>> + float val = fabsf(inst->src[i].fixed_hw_reg.dw1.f);
>> + struct imm *imm = find_imm(&table, val);
>> +
>> + if (imm) {
>> + bblock_t *intersection = cfg_t::intersect(block, imm->block);
>> + if (intersection != imm->block)
>> + imm->inst = NULL;
>> + imm->block = intersection;
>> + imm->uses->push_tail(link(const_ctx, &inst->src[i]));
>> + imm->uses_by_coissue += could_coissue(inst);
>> + imm->last_use_ip = ip;
>> + } else {
>> + imm = new_imm(&table, const_ctx);
>> + imm->block = block;
>> + imm->inst = inst;
>> + imm->uses = new(const_ctx) exec_list();
>> + imm->uses->push_tail(link(const_ctx, &inst->src[i]));
>> + imm->val = val;
>> + imm->uses_by_coissue = could_coissue(inst);
>> + imm->inserted = false;
>> + imm->first_use_ip = ip;
>> + imm->last_use_ip = ip;
>> + }
>> + }
>> + }
>> +
>> + /* Remove constants from the table that don't have enough uses to make them
>> + * profitable to store in a register.
>> + */
>> + for (int i = 0; i < table.len;) {
>> + struct imm *imm = &table.imm[i];
>> +
>> + if (imm->uses_by_coissue < 4) {
>> + table.imm[i] = table.imm[table.len - 1];
>> + table.len--;
>> + continue;
>> + }
>> + i++;
>
> Whatever reason i++ had for being here doesn't appear to exist anymore -
> move it inside the for(...)?
I think it is right, actually.
If we find an immediate we don't want to promote, we overwrite it with
the last element in the table and then continue without incrementing i
so that we get the same i value again.
>
>> + }
>> + if (table.len == 0) {
>> + ralloc_free(const_ctx);
>> + return;
>> + }
>> + if (cfg->num_blocks != 1)
>> + qsort(table.imm, table.len, sizeof(struct imm), compare);
>> +
>> +
>
> /* Insert MOVs to load the constant values into GRFs. */
>
>> + fs_reg reg(GRF, virtual_grf_alloc(dispatch_width / 8));
>> + reg.stride = 0;
>> + for (int i = 0; i < table.len; i++) {
>> + struct imm *imm = &table.imm[i];
>> + if (imm->inserted)
>> + continue;
>> +
>> + fs_inst *mov = MOV(reg, fs_reg(imm->val));
>> + if (imm->inst) {
>> + imm->inst->insert_before(imm->block, mov);
>> + } else {
>> + backend_instruction *inst = imm->block->last_non_control_flow_inst();
>> + inst->insert_after(imm->block, mov);
>> + }
>> + imm->reg = reg.reg;
>> + imm->subreg_offset = reg.subreg_offset;
>> +
>> + reg.subreg_offset += sizeof(float);
>> + if ((unsigned)reg.subreg_offset == dispatch_width * sizeof(float)) {
>> + reg.reg = virtual_grf_alloc(dispatch_width / 8);
>> + reg.subreg_offset = 0;
>> + }
>> + }
>> +
>
> /* Rewrite the immediate sources to refer to the new GRFs. */
>
>> + for (int i = 0; i < table.len; i++) {
>> + foreach_list_typed(reg_link, link, link, table.imm[i].uses) {
>> + fs_reg *reg = link->reg;
>> + reg->file = GRF;
>> + reg->reg = table.imm[i].reg;
>> + reg->subreg_offset = table.imm[i].subreg_offset;
>> + reg->stride = 0;
>> + reg->negate = signbit(reg->fixed_hw_reg.dw1.f) !=
>> + signbit(table.imm[i].val);
>> + assert(fabsf(reg->fixed_hw_reg.dw1.f) == table.imm[i].val);
>> + }
>> + }
>> +
>> + ralloc_free(const_ctx);
>> + invalidate_live_intervals();
>> +}
>
> This pass looks solid! With comments and the few small changes,
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
Thanks a bunch Ken. I really appreciate it!
More information about the mesa-dev
mailing list