[Mesa-dev] [PATCH v3] i965/vec4: Change types as needed to propagate source modifiers using from instruction
Jason Ekstrand
jason at jlekstrand.net
Sat Sep 19 07:34:44 PDT 2015
On Sep 18, 2015 10:12 AM, "Alejandro Piñeiro" <apinheiro at 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.
> 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 << 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150919/91bd25fd/attachment-0001.html>
More information about the mesa-dev
mailing list