On Sep 18, 2015 10:12 AM, "Alejandro Piñeiro" <<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>> wrote: > > SEL and MOV instructions, as long as they don't have source modifiers, are > just copying bits around. So those kind of instruction could be propagated > even if there are type mismatches. This is needed because NIR generates > integer SEL and MOV instructions whenever it doesn't know what else to > generate. > > This commit adds support for copy propagation using previous instruction > as reference. > > v2: using 'arg' index to get the from inst was wrong, as pointed > by Jason Ekstrand > v3: rebased against last change on the previous patch of theh series > --- > > v3 is just a rebase against the first patch on the series. I also > realized that yesterday I did a mess when I sent the v2 of this > patch (answering to the wrong email, not including v3 on the subject), > so I will copy the text I had on that email here. Sorry for the noise. > > From the review: > >> if (has_source_modifiers && > >> value.type != inst->src[arg].type && > >> - !can_change_source_types(inst)) > >> + !can_change_source_types(inst) && > >> + !can_change_source_types(entry->inst[arg])) > > > This isn't right. The entry->inst array is indexed on vec4 component > > but arg is the argument of the instruction. I think what you want to > > do is add something to the loop above to loop over 0...3 and check > > them all. > > Yes you are right. At that moment I misunderstood 'arg' as the channel > being used at that moment, and as piglit didn't correct me with a > regression I never realized that I was wrong. This v2 computes > the instruction we are looking for corrrectly, with the a loop > as suggested. > > > Also, how this is different from has_source_modifiers? > > It is different because at that moment has_source_modifiers has > been computed using the src_reg value that had their modifiers > changed to the ones at the current string: > > if (inst->src[arg].abs) { > value.negate = false; > value.abs = true; > } > if (inst->src[arg].negate) > value.negate = !value.negate; > > bool has_source_modifiers = value.negate || value.abs; > > While can_change_source_types with the from instruction is > using the original ones, and checking that is a mov, so "safe" > to going on with the propagation. > > That means that there is an alternative to track the instructions > at copy_entry. I could maintain a copy of value without being > modified, or compute has_source_modifiers (ie: original_has_source_modifiers). I think I would prefer that if we can make it work. > But I would still need to check if the opcode is a MOV on the from > instruction, so I would need to track opcode on the copy_entry struct. > Tracking inst also have the advantage that I can reuse the > function can_change_source_types. The copy_entry struct shouldn't get filled out. In particular, we only set the source if is_direct_copy() returns true. > > > Obviously, it is (the shader-db numbers say so) but I'm not seeing it. > > Could you please provide an example. > > Ok. On a shader like this: > > uniform vec4 inData; > > void main(void) > { > gl_Position = gl_Vertex - inData; > } > > VS-0001-00-start is like this: > 0: mov vgrf0.0:F, attr0.xyzw:F > 1: mov vgrf1.0:UD, u0.xyzw:UD > 2: add vgrf2.0:F, vgrf0.xyzw:F, -vgrf1.xyzw:F > 3: mov m2:D, 0D > 4: mov m3:F, vgrf2.xyzw:F > 5: vs_urb_write (null):UD > > When we are at instruction #2, has_source_modifiers is true > even for the value u0 because as I mentioned, it is copying the > modifiers at the same instruction (the modifiers of > vgrf1 at #2). But in any case, it is "safe" to do the copy > propagation from instruction #1 to instruction #2. Thanks, that makes sense. --Jason > > .../drivers/dri/i965/brw_vec4_copy_propagation.cpp | 47 ++++++++++++++++------ > 1 file changed, 35 insertions(+), 12 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp > index 1522eea..dabc25f 100644 > --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp > +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp > @@ -39,6 +39,7 @@ namespace brw { > > struct copy_entry { > src_reg *value[4]; > + vec4_instruction *inst[4]; > int saturatemask; > }; > > @@ -251,7 +252,8 @@ try_constant_propagate(const struct brw_device_info *devinfo, > static bool > can_change_source_types(vec4_instruction *inst) > { > - return inst->dst.type == inst->src[0].type && > + return inst != NULL && > + inst->dst.type == inst->src[0].type && > !inst->src[0].abs && !inst->src[0].negate && !inst->saturate && > (inst->opcode == BRW_OPCODE_MOV || > (inst->opcode == BRW_OPCODE_SEL && > @@ -271,6 +273,8 @@ try_copy_propagate(const struct brw_device_info *devinfo, > * vectors better. > */ > src_reg value = *entry->value[0]; > + vec4_instruction *from_inst = entry->inst[0]; > + > for (int i = 1; i < 4; i++) { > /* This is equals() except we don't care about the swizzle. */ > if (value.file != entry->value[i]->file || > @@ -283,6 +287,13 @@ try_copy_propagate(const struct brw_device_info *devinfo, > } > } > > + for (int i = 1; i < 4; i++) { > + if (from_inst != entry->inst[i]) { > + from_inst = NULL; > + break; > + } > + } > + > /* Compute the swizzle of the original register by swizzling the > * component loaded from each value according to the swizzle of > * operand we're going to change. > @@ -322,7 +333,8 @@ try_copy_propagate(const struct brw_device_info *devinfo, > > if (has_source_modifiers && > value.type != inst->src[arg].type && > - !can_change_source_types(inst)) > + !can_change_source_types(inst) && > + !can_change_source_types(from_inst)) > return false; > > if (has_source_modifiers && > @@ -340,7 +352,8 @@ try_copy_propagate(const struct brw_device_info *devinfo, > * instead. See also resolve_ud_negate(). > */ > if (value.negate && > - value.type == BRW_REGISTER_TYPE_UD) > + value.type == BRW_REGISTER_TYPE_UD && > + !can_change_source_types(from_inst)) > return false; > > /* Don't report progress if this is a noop. */ > @@ -378,17 +391,25 @@ try_copy_propagate(const struct brw_device_info *devinfo, > > if (has_source_modifiers && > value.type != inst->src[arg].type) { > - /* We are propagating source modifiers from a MOV with a different > - * type. If we got here, then we can just change the source and > - * destination types of the instruction and keep going. > + /* We are propagating source modifiers from a safe instruction with a > + * different type. If we got here, then we can just change the source > + * and destination types of the current instruction or the instruction > + * from we are propagating. > */ > - assert(can_change_source_types(inst)); > - for (int i = 0; i < 3; i++) { > - inst->src[i].type = value.type; > + assert(can_change_source_types(inst) || > + can_change_source_types(from_inst)); > + > + if (can_change_source_types(inst)) { > + for (int i = 0; i < 3; i++) { > + inst->src[i].type = value.type; > + } > + inst->dst.type = value.type; > + } else { > + value.type = inst->src[arg].type; > } > - inst->dst.type = value.type; > - } else > + } else { > value.type = inst->src[arg].type; > + } > inst->src[arg] = value; > return true; > } > @@ -441,7 +462,7 @@ vec4_visitor::opt_copy_propagation(bool do_constant_prop) > for (c = 0; c < 4; c++) { > int channel = BRW_GET_SWZ(inst->src[i].swizzle, c); > entry.value[c] = entries[reg].value[channel]; > - > + entry.inst[c] = entries[reg].inst[channel]; > /* If there's no available copy for this channel, bail. > * We could be more aggressive here -- some channels might > * not get used based on the destination writemask. > @@ -486,6 +507,7 @@ vec4_visitor::opt_copy_propagation(bool do_constant_prop) > entries[reg].value[i] = direct_copy ? &inst->src[0] : NULL; > entries[reg].saturatemask |= > inst->saturate && direct_copy ? 1 < > + entries[reg].inst[i] = direct_copy ? inst : NULL; > } > } > > @@ -500,6 +522,7 @@ vec4_visitor::opt_copy_propagation(bool do_constant_prop) > if (is_channel_updated(inst, entries[i].value, j)) { > entries[i].value[j] = NULL; > entries[i].saturatemask &= ~(1 << j); > + entries[i].inst[j] = NULL; > } > } > } > -- > 2.1.4 > > _______________________________________________ > 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">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>