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?
> +
> + 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
*/
> +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(...)?
> + }
> + 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 <kenn...@whitecape.org>
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
