[Mesa-dev] [PATCH v4] i965/vec4: Change types as needed to propagate source modifiers using from instruction
Jason Ekstrand
jason at jlekstrand.net
Mon Sep 21 09:27:42 PDT 2015
On Sun, Sep 20, 2015 at 3:00 PM, Alejandro Piñeiro <apinheiro at igalia.com> wrote:
> 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 MOV instructions whenever it doesn't know what
> else to generate.
>
> This commit adds support for copy propagation using previous (from)
> instruction as reference.
>
> Shader-db results for vec4 programs on Haswell:
> total instructions in shared programs: 1683959 -> 1669037 (-0.89%)
> instructions in affected programs: 746238 -> 731316 (-2.00%)
> helped: 6264
> HURT: 30
>
> 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 the series
> v4: don't need to track instructions on struct copy_entry, as we
> only set the source on a direct copy, as pointed by Jason
> ---
> .../drivers/dri/i965/brw_vec4_copy_propagation.cpp | 36 ++++++++++++++++------
> 1 file changed, 26 insertions(+), 10 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..193c7d0 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> @@ -271,6 +271,8 @@ try_copy_propagate(const struct brw_device_info *devinfo,
> * vectors better.
> */
> src_reg value = *entry->value[0];
> + bool from_inst_has_source_modifiers = value.negate || value.abs;
> +
> 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 ||
> @@ -311,6 +313,10 @@ try_copy_propagate(const struct brw_device_info *devinfo,
> if (inst->src[arg].negate)
> value.negate = !value.negate;
>
> + /* has_source_modifiers is using the modifiers at the current instruction,
> + * while from_inst_source_modifiers is using the modifiers of the source
> + * at the instruction the source comes from
> + */
> bool has_source_modifiers = value.negate || value.abs;
>
> /* gen6 math and gen7+ SENDs from GRFs ignore source modifiers on
> @@ -322,7 +328,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) &&
> + from_inst_has_source_modifiers)
I don't think this quite the condition we want. Really, what we want is
if (from_inst_has_source_modifiers &&
value.type != inst->src[arg].type &&
!can_change_source_types(inst))
Whether or not the final thing will have source modifiers is
irrelevant. The only thing that matters is that we have some that
need propagating (from_inst_has_source_modifiers) but we cant (other
two conditions).
> return false;
>
> if (has_source_modifiers &&
> @@ -340,7 +347,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 &&
> + from_inst_has_source_modifiers)
Why is this here? I think this check should just be moved up to
before we modify value. Maybe as its own patch.
> return false;
>
> /* Don't report progress if this is a noop. */
> @@ -378,17 +386,25 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>
> if (has_source_modifiers &&
from_inst_has_source_modifiers
Sorry to make this go through yet another re-spin, but having looked
through vec4 copy propagation yet again and finally understanding what
it's doing, I think we need to refactor it a bit. In the FS copy
propagation pass, we wait until the end to build the "final value" and
we should probably do that in vec4 as well. In particular, it should
look something like this in the end:
1) Build up the value we are propagating as if it were the source of a
single MOV:
- The first for loop to ensure that all values touch the same register
- The loop to figure out what the value swizzle looks like but
without composing it with inst->src[arg].swizzle
2) Check that we can propagate that value
- Check the register file
- Check for the can_change_source_mods condition
- etc...
(Really, all of the checks can be done without the final value. Some
of them make a little more sense with the final value such as the
can_do_source_mods() check, but if you can't do source modifiers you
can assume that there aren't any there already so has_source_modifiers
is the same as from_inst_has_source_modifiers.)
3) Build the final value;
- Compose swizzlese
- Compute final source modifiers
Whether step 3 happens by modifying value or by directly modifying
inst->src[arg], I don't really care. Do whatever feels best.
In the end, the two passes (vec4 and fs) should look very similar.
Does this all make sense and sound reasonable? I trust you to make a
decent series (maybe only one patch, I don't know) out of it.
Thanks for working on this!
--Jason
> 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) ||
> + !from_inst_has_source_modifiers);
> +
> + 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;
> }
> --
> 2.1.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list