[Mesa-dev] [PATCH 2/3] i965/fs: Make emit_uniformize a no-op for immediates

[Francisco Jerez]

Neil Roberts <neil at linux.intel.com> writes:

> Just a thought, would it be better to move this check into the
> eliminate_find_live_channel optimisation? That way it could catch
> sources that become immediates through later optimisations. One problem
> with this that I've seen before is that eliminating the
> FIND_LIVE_CHANNEL doesn't cause the subsequent BROADCAST to be
> eliminated because the copy propagation doesn't work.

I believe in this particular case the BROADCAST instruction should
already be eliminated (by opt_algebraic), because its first source is an
immediate, so this optimization would in fact be redundant with
opt_algebraic+constant propagation if it weren't because the latter is
unable to handle scalar copies.  Another possibility that would likely
be more general than this (because it would handle cases in which, as
Neil said, the argument becomes an immediate only after optimization,
and because it would also avoid the issue with liveness analysis
extending the live ranges of scalar values incorrectly [1]) would be to
extend the destination register of the BROADCAST instructions to a
vector [2] and then add a case to the switch statement of
try_constant_propagate() so that the immediate MOV resulting from
opt_algebraic is propagated into surface instructions (see attachment).

> I made this patch a while ago but I never posted it anywhere because
> it's a of a kludge and it would probably be better to fix the copy
> propagation:
>
> https://github.com/bpeel/mesa/commit/e4c3286075f891f466fb8558106d2aaa
>
Heh, yeah, I'd rather fix copy propagation instead, which I believe will
become much easier with the use-def-chain analysis pass I'm working on.

> Either way though I don't think it would do any harm to have Kristian's
> patch as well even if we did improve elimintate_find_live_channel so it
> is:
>
> Reviewed-by: Neil Roberts <neil at linux.intel.com>
>
> - Neil
>
> Kristian Høgsberg Kristensen <krh at bitplanet.net> writes:
>
>> An immdiate is already uniform so just return it up front. Without this,
>> brw_fs_surface_builder ends up passing immediate surface indices through
>> SHADER_OPCODE_BROADCAST. This writes to a stride 0 dst, which we can't
>> constant propagate out of, and further, we don't constant propagate into
>> the typed/untype read/write opcodes at all.  The end result is that all
>> typed/untyped read/write/atomics end up as indirect sends.
>>
>> Code generation should always produce either an immediate or an actual
>> indirect surface index, so we can fix this by just special casing
>> immediates in emit_uniformize.
>>
>> Signed-off-by: Kristian Høgsberg Kristensen <krh at bitplanet.net>
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs_builder.h | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_builder.h b/src/mesa/drivers/dri/i965/brw_fs_builder.h
>> index df10a9d..98ce71e 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_builder.h
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_builder.h
>> @@ -394,6 +394,9 @@ namespace brw {
>>           const dst_reg chan_index = component(vgrf(BRW_REGISTER_TYPE_UD), 0);
>>           const dst_reg dst = component(vgrf(src.type), 0);
>>  
>> +         if (src.file == IMM)
>> +            return src;
>> +
>>           ubld.emit(SHADER_OPCODE_FIND_LIVE_CHANNEL, chan_index);
>>           ubld.emit(SHADER_OPCODE_BROADCAST, dst, src, chan_index);
>>  
>> -- 
>> 2.6.2
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> 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-October/097085.html
[2] http://lists.freedesktop.org/archives/mesa-dev/attachments/20151014/25dd38dc/attachment.patch

-------------- next part --------------
A non-text attachment was scrubbed...
Name: i965_surface_constant_propagate.patch
Type: text/x-diff
Size: 1619 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151019/6de685f9/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151019/6de685f9/attachment-0001.sig>