On Wed, Dec 17, 2014 at 8:41 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
>
>
> 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
Nvm, I was reviewing this from your freedesktop branch, which had this
patch already applied. Yeah, splitting it up would be nice though.
>
>>
>>
>> > --- 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
