[Mesa-dev] [PATCH v5] i965/vec4: refactor brw_vec4_copy_propagation.
Alejandro Piñeiro
apinheiro at igalia.com
Tue Sep 22 09:17:17 PDT 2015
On 22/09/15 18:09, Jason Ekstrand wrote:
> On Tue, Sep 22, 2015 at 8:06 AM, Alejandro Piñeiro <apinheiro at igalia.com> wrote:
>> Now it is more similar to brw_fs_copy_propagation, with three
>> clear stages:
>>
>> 1) Build up the value we are propagating as if it were the source of a
>> single MOV:
>> 2) Check that we can propagate that value
>> 3) Build the final value
>>
>> Previously everything was somewhat messed up, making the
>> implementation on some specific cases, like knowing if you can
>> propagate from a previous instruction even with type mismatches even
>> messier (for example, with the need of maintaining more of one
>> has_source_modifiers). The refactoring clears stuff, and gives
>> support to this mentioned use case without doing anything extra
>> (for example, only one has_source_modifiers is used).
>>
>> Shader-db results for vec4 programs on Haswell:
>> total instructions in shared programs: 1683842 -> 1669037 (-0.88%)
>> instructions in affected programs: 739837 -> 725032 (-2.00%)
>> helped: 6237
>> HURT: 0
>>
>> 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
>> v5: change the approach for a refactoring, as pointed by Jason
>> ---
>>
>> Just the refactoring suggested at v4 review is enough to get the use
>> case was dealing with at the beginning solved. It would be hard
>> to split this patch in two (refactoring+solve issue).
>>
>> Tested with piglit without any regression. Needed to update shader-db
>> numbers after last Matt Turner's improvements. I think that the fact
>> of going from 30 HURT (v4) to 0 (v5) is Matt's merit.
>>
>> I added comments to clearly mark the different stages mentioned at
>> v4 review (1, 2 and 3), to make the review easier, but if the patch
>> gets approved, they can go away.
> I'd like to keep them only without the "1)", "2)", etc.
Ok.
>> The change itself doesn't explain clearly how it got solved, but the
>> resulting code is clearer. Thanks for the thoroughly review.
>>
>> .../drivers/dri/i965/brw_vec4_copy_propagation.cpp | 28 ++++++++++++++--------
>> 1 file changed, 18 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..8a0bd72 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>> @@ -265,6 +265,9 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>> vec4_instruction *inst,
>> int arg, struct copy_entry *entry)
>> {
>> + /* 1) Build up the value we are propagating as if it were the source of a
>> + * single MOV
>> + */
>> /* For constant propagation, we only handle the same constant
>> * across all 4 channels. Some day, we should handle the 8-bit
>> * float vector format, which would let us constant propagate
> Can we please kill this comment while we're here? It seems to have
> gotten copied+pasted from constant propagation and makes no sense in
> this context.
Ok.
>
> With that, and the updated comments,
>
> Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>
>
> I'm also running it through our CI system as I type. I'll let you
> know how that goes when I get to my office in about an hour.
Ok. I will wait before pushing. I should be still around.
> Do you have push access yet? If not, I can make the trivial changes
> and push this for you.
> --Jason
Yes, I have push access, so I can make the changes myself. As mentioned
I will wait for the CI system outcome.
>
>> @@ -291,9 +294,9 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>> for (int i = 0; i < 4; i++) {
>> s[i] = BRW_GET_SWZ(entry->value[i]->swizzle, i);
>> }
>> - value.swizzle = brw_compose_swizzle(inst->src[arg].swizzle,
>> - BRW_SWIZZLE4(s[0], s[1], s[2], s[3]));
>> + value.swizzle = BRW_SWIZZLE4(s[0], s[1], s[2], s[3]);
>>
>> + /* 2) Check that we can propagate that value */
>> if (value.file != UNIFORM &&
>> value.file != GRF &&
>> value.file != ATTR)
>> @@ -304,13 +307,6 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>> return false;
>> }
>>
>> - 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;
>>
>> /* gen6 math and gen7+ SENDs from GRFs ignore source modifiers on
>> @@ -376,6 +372,16 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>> }
>> }
>>
>> + /* 3) Build the final value */
>> + if (inst->src[arg].abs) {
>> + value.negate = false;
>> + value.abs = true;
>> + }
>> + if (inst->src[arg].negate)
>> + value.negate = !value.negate;
>> +
>> + value.swizzle = brw_compose_swizzle(inst->src[arg].swizzle,
>> + value.swizzle);
>> if (has_source_modifiers &&
>> value.type != inst->src[arg].type) {
>> /* We are propagating source modifiers from a MOV with a different
>> @@ -387,8 +393,10 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>> inst->src[i].type = value.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
--
Alejandro Piñeiro (apinheiro at igalia.com)
More information about the mesa-dev
mailing list