[Mesa-dev] [PATCH v4] i965/vec4: Change types as needed to propagate source modifiers using from instruction
Alejandro Piñeiro
apinheiro at igalia.com
Mon Sep 21 10:21:31 PDT 2015
On 21/09/15 18:27, Jason Ekstrand wrote:
> 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).
Yes, true, you can simplify the condition that way. Make things clearer.
>
>> 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.
Because it is really usual that the default types are UD. So on an
assembly code like this:
0: mov vgrf1.0:UD, u0.xyzw:UD
1: add m3:F, attr0.xyzw:F, -vgrf1.xyzw:F
if we don't modify that condition to take into account
from_inst_has_source_modifiers, it makes to bail out the previous scenario.
>> 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.
Not sure if it is fair the comparison with FS right now. For the
equivalent shader, FS have the correct types on those MOVs, that was how
the the original patch (this [1]) tried to solve the situation. This
patch is trying to solve this issue on copy-propagation, something FS
doesn't need to.
> 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.
I would need some thought and testing on this. Will start to work based
on this.
> In the end, the two passes (vec4 and fs) should look very similar.
> Does this all make sense and sound reasonable?
Again, fs doesn't need to cover the use case this patch is trying to
solve. And it is not trying to deal with it either. So at least at some
point, there will be some differences between one and the other. In any
case, will try to see what can I do with the detailed template before.
> I trust you to make a
> decent series (maybe only one patch, I don't know) out of it.
>
> Thanks for working on this!
Thanks for all the quick reviews and patience.
> --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
[1]
http://lists.freedesktop.org/archives/mesa-dev/2015-September/094547.html
--
Alejandro Piñeiro (apinheiro at igalia.com)
More information about the mesa-dev
mailing list