<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Dec 17, 2014 at 5:30 PM, Connor Abbott <<a href="mailto:cwabbott0@gmail.com" target="_blank">cwabbott0@gmail.com</a>> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Dec 16, 2014 at 1:05 AM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> 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. </blockquote><div> </div><div>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". </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > --- 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 > </div></div>> _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div></div></div>