On 19/09/15 16:34, Jason Ekstrand wrote:
>
>
> On Sep 18, 2015 10:12 AM, "Alejandro Piñeiro" <apinhe...@igalia.com
> <mailto:apinhe...@igalia.com>> 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.
>
Ok.
> > 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.
>
Oh true, if I have a source, that means that it came from a MOV. Good catch.
Thanks for the hints.
> >
> > > 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 << i : 0;
> > + 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
> > mesa-dev@lists.freedesktop.org <mailto:mesa-dev@lists.freedesktop.org>
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
--
Alejandro Piñeiro (apinhe...@igalia.com)
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
