On Wed, Dec 17, 2014 at 5:30 PM, Connor Abbott <cwabbo...@gmail.com> wrote:
>
> On Tue, Dec 16, 2014 at 1:05 AM, Jason Ekstrand <ja...@jlekstrand.net>
> wrote:
> > ---
> > src/glsl/nir/nir_from_ssa.c | 40 ++++++++++++++++++++-----
> > src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 50
> +++++++++++++++++++++-----------
> > 2 files changed, 65 insertions(+), 25 deletions(-)
> >
> > diff --git a/src/glsl/nir/nir_from_ssa.c b/src/glsl/nir/nir_from_ssa.c
> > index 62a54fe..9311bda 100644
>
> It would be nice if you could squash all the following nir_from_ssa.c
> changes into "nir: Add a better out-of-ssa pass", since nir_from_ssa.c
> already has some stuff about special-casing SSA constants so this is
> rather confusing.
>
Certainly, this could be split into two patches: The first one makes i965
ssa-const safe, and the second doesn't lower load_const SSA values.
However, for the sake of the history, I'd like to keep it seperate from
from_ssa. Also, load_const is never mentioned in nir_from_ssa before this
patch, so I'm not sure what you mean by "already have special-casing".
--Jason
>
> > --- a/src/glsl/nir/nir_from_ssa.c
> > +++ b/src/glsl/nir/nir_from_ssa.c
> > @@ -469,6 +469,12 @@ get_register_for_ssa_def(nir_ssa_def *def, struct
> from_ssa_state *state)
> > if (entry) {
> > return (nir_register *)entry->data;
> > } else {
> > + /* We leave load_const SSA values alone. They act as immediates
> to
> > + * the backend. If it got coalesced into a phi, that's ok.
> > + */
> > + if (def->parent_instr->type == nir_instr_type_load_const)
> > + return NULL;
> > +
> > nir_register *reg = nir_local_reg_create(state->impl);
> > reg->name = def->name;
> > reg->num_components = def->num_components;
> > @@ -486,12 +492,18 @@ rewrite_ssa_src(nir_src *src, void *void_state)
> > struct from_ssa_state *state = void_state;
> >
> > if (src->is_ssa) {
> > - /* We don't need to remove it from the uses set because that is
> going
> > - * away. We just need to add it to the one for the register. */
> > nir_register *reg = get_register_for_ssa_def(src->ssa, state);
> > +
> > + if (reg == NULL) {
> > + assert(src->ssa->parent_instr->type ==
> nir_instr_type_load_const);
> > + return true;
> > + }
> > +
> > memset(src, 0, sizeof *src);
> > src->reg.reg = reg;
> >
> > + /* We don't need to remove it from the uses set because that is
> going
> > + * away. We just need to add it to the one for the register. */
> > _mesa_set_add(reg->uses, _mesa_hash_pointer(state->instr),
> state->instr);
> > }
> >
> > @@ -504,10 +516,16 @@ rewrite_ssa_dest(nir_dest *dest, void *void_state)
> > struct from_ssa_state *state = void_state;
> >
> > if (dest->is_ssa) {
> > + nir_register *reg = get_register_for_ssa_def(&dest->ssa, state);
> > +
> > + if (reg == NULL) {
> > + assert(dest->ssa.parent_instr->type ==
> nir_instr_type_load_const);
> > + return true;
> > + }
> > +
> > _mesa_set_destroy(dest->ssa.uses, NULL);
> > _mesa_set_destroy(dest->ssa.if_uses, NULL);
> >
> > - nir_register *reg = get_register_for_ssa_def(&dest->ssa, state);
> > memset(dest, 0, sizeof *dest);
> > dest->reg.reg = reg;
> >
> > @@ -534,7 +552,6 @@ resolve_registers_block(nir_block *block, void
> *void_state)
> > instr->type == nir_instr_type_phi) {
> > nir_instr_remove(instr);
> > ralloc_steal(state->dead_ctx, instr);
> > - continue;
> > }
> > }
> > state->instr = NULL;
> > @@ -543,11 +560,18 @@ resolve_registers_block(nir_block *block, void
> *void_state)
> > if (following_if && following_if->condition.is_ssa) {
> > nir_register *reg =
> get_register_for_ssa_def(following_if->condition.ssa,
> > state);
> > - memset(&following_if->condition, 0, sizeof
> following_if->condition);
> > - following_if->condition.reg.reg = reg;
> > + if (reg) {
> > + memset(&following_if->condition, 0, sizeof
> following_if->condition);
> > + following_if->condition.reg.reg = reg;
> >
> > - _mesa_set_add(reg->if_uses, _mesa_hash_pointer(following_if),
> > - following_if);
> > + _mesa_set_add(reg->if_uses, _mesa_hash_pointer(following_if),
> > + following_if);
> > + } else {
> > + /* FIXME: We really shouldn't hit this. We should be doing
> > + * constant control flow propagation.
> > + */
> > + assert(following_if->condition.ssa->parent_instr->type ==
> nir_instr_type_load_const);
> > + }
> > }
> >
> > return true;
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > index 3ec2fa6..019d649 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > @@ -981,25 +981,37 @@ fs_visitor::nir_emit_alu(nir_alu_instr *instr)
> > fs_reg
> > fs_visitor::get_nir_src(nir_src src)
> > {
> > - fs_reg reg;
> > - if (src.reg.reg->is_global)
> > - reg = nir_globals[src.reg.reg->index];
> > - else
> > - reg = nir_locals[src.reg.reg->index];
> > + if (src.is_ssa) {
> > + assert(src.ssa->parent_instr->type == nir_instr_type_load_const);
> > + nir_load_const_instr *load =
> nir_instr_as_load_const(src.ssa->parent_instr);
> > + fs_reg reg(GRF, virtual_grf_alloc(src.ssa->num_components),
> > + BRW_REGISTER_TYPE_D);
> >
> > - /* to avoid floating-point denorm flushing problems, set the type by
> > - * default to D - instructions that need floating point semantics
> will set
> > - * this to F if they need to
> > - */
> > - reg.type = BRW_REGISTER_TYPE_D;
> > - reg.reg_offset = src.reg.base_offset;
> > - if (src.reg.indirect) {
> > - reg.reladdr = new(mem_ctx) fs_reg();
> > - *reg.reladdr = retype(get_nir_src(*src.reg.indirect),
> > - BRW_REGISTER_TYPE_D);
> > - }
> > + for (unsigned i = 0; i < src.ssa->num_components; ++i)
> > + emit(MOV(offset(reg, i), fs_reg(load->value.i[i])));
> >
> > - return reg;
> > + return reg;
> > + } else {
> > + fs_reg reg;
> > + if (src.reg.reg->is_global)
> > + reg = nir_globals[src.reg.reg->index];
> > + else
> > + reg = nir_locals[src.reg.reg->index];
> > +
> > + /* to avoid floating-point denorm flushing problems, set the type
> by
> > + * default to D - instructions that need floating point semantics
> will set
> > + * this to F if they need to
> > + */
> > + reg.type = BRW_REGISTER_TYPE_D;
> > + reg.reg_offset = src.reg.base_offset;
> > + if (src.reg.indirect) {
> > + reg.reladdr = new(mem_ctx) fs_reg();
> > + *reg.reladdr = retype(get_nir_src(*src.reg.indirect),
> > + BRW_REGISTER_TYPE_D);
> > + }
> > +
> > + return reg;
> > + }
> > }
> >
> > fs_reg
> > @@ -1652,6 +1664,10 @@ fs_visitor::nir_emit_texture(nir_tex_instr *instr)
> > void
> > fs_visitor::nir_emit_load_const(nir_load_const_instr *instr)
> > {
> > + /* Bail on SSA constant loads. These are used for immediates. */
> > + if (instr->dest.is_ssa)
> > + return;
> > +
> > fs_reg dest = get_nir_dest(instr->dest);
> > dest.type = BRW_REGISTER_TYPE_UD;
> > if (instr->array_elems == 0) {
> > --
> > 2.2.0
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
