[Mesa-dev] [PATCH] nir: propagate known constant values into the if-then branch

[Timothy Arceri]

On 23/1/19 3:27 pm, Caio Marcelo de Oliveira Filho wrote:
> Hi,
> 
>>> The different problem I've found was that uses in the phi instruction
>>> after the then/else blocks was being replaced, causing churn in the
>>> optimizations further on.  As a hack, I've ignored phi instructions in
>>> the use loop above.  HURTs are gone and HELPs continued.
>>>
>>> The real issue seems to be use_block not being what we expect when we
>>> have a use_src from a phi instruction.  For example, in the snippet
>>> below
>>>
>>>       vec1 1 ssa_2543 = ieq ssa_7429, ssa_6
>>>       /* succs: block_9 block_10 */
>>>       if ssa_2543 {
>>>               block block_9:
>>>               /* preds: block_8 */
>>>               vec2 32 ssa_2550 = vec2 ssa_7744, ssa_7745
>>>               vec4 32 ssa_2553 = txl ssa_468 (texture_deref), ssa_468 (sampler_deref), ssa_2550 (coord), ssa_6 (lod),
>>>               vec1 32 ssa_7434 = imov ssa_2553.w
>>>               /* succs: block_11 */
>>>       } else {
>>>               block block_10:
>>>               /* preds: block_8 */
>>>               /* succs: block_11 */
>>>       }
>>>       block block_11:
>>>       /* preds: block_9 block_10 */
>>>       vec1 32 ssa_7433 = phi block_9: ssa_7420, block_10: ssa_7420
>>>       vec1 32 ssa_7436 = phi block_9: ssa_7434, block_10: ssa_7423
>>>       vec1 32 ssa_7439 = phi block_9: ssa_7426, block_10: ssa_7426
>>>       vec1 32 ssa_7442 = phi block_9: ssa_7429, block_10: ssa_7429
>>>
>>> the use_src that is part of the instruction "... ssa_7442 = phi ...",
>>> was being considered use_block == 9 (the 'then'-block), so replace was
>>> happening.
>>
>> I'm not sure I see the problem, replacing the phi src is valid. The question
>> is why is it causing HURT?
> 
> What happens is that the phi becomes
> 
>      vec1 32 ssa_7442 = phi block_9: ssa_6, block_10: ssa_7429
> 
> instead of remaining
> 
>      vec1 32 ssa_7442 = phi block_9: ssa_7429, block_10: ssa_7429
> 
> and the next pass that cause effect (in this case) is
> nir_opt_remove_phis, which will not be able to remove this phi.
> Further passes will still keep this phi around, and it sticks until
> SSA work is done.  This is the SSA form (replacing the phi, 23 NIR
> instructions)
> 
>      block block_11:
>      /* preds: block_9 block_10 */
>      vec1 32 ssa_127 = phi block_9: ssa_126, block_10: ssa_101
>      vec1 32 ssa_128 = phi block_9: ssa_0, block_10: ssa_104    <<<<<
>      vec1 32 ssa_129 = fadd -ssa_96, ssa_122
>      vec1 32 ssa_130 = b32csel ssa_106, ssa_122, ssa_129
>      vec1 32 ssa_131 = fadd -ssa_96, ssa_127
>      vec1 32 ssa_132 = ine32 ssa_128, ssa_0
>      vec1 32 ssa_133 = b32csel ssa_132, ssa_127, ssa_131

Looks like the two above instructions and the phi could possibly be 
replaced by a single phi:

vec1 32 ssa_133 = phi block_9: ssa_131, block_10: ssa_127

>      vec1 32 ssa_134 = fge32 abs(ssa_133), ssa_95
>      vec1 32 ssa_135 = b2i32 ssa_134
>      vec1 32 ssa_136 = imov -ssa_135
>      vec1 32 ssa_137 = ffma -ssa_78, ssa_6, ssa_107
>      vec1 32 ssa_138 = fge32 abs(ssa_130), ssa_95
>      vec1 32 ssa_139 = b32csel ssa_138, ssa_107, ssa_137
>      vec1 32 ssa_140 = ffma -ssa_79, ssa_6, ssa_109
>      vec1 32 ssa_141 = b32csel ssa_138, ssa_109, ssa_140
>      vec1 32 ssa_142 = iand ssa_134, ssa_138
>      vec1 32 ssa_143 = b2i32 ssa_142
>      vec1 32 ssa_144 = ffma ssa_78, ssa_6, ssa_113
>      vec1 32 ssa_145 = b32csel ssa_134, ssa_113, ssa_144
>      vec1 32 ssa_146 = ffma ssa_79, ssa_6, ssa_115
>      vec1 32 ssa_147 = b32csel ssa_134, ssa_115, ssa_146
>      vec1 32 ssa_148 = inot -ssa_143
>      vec1 32 ssa_149 = ine32 ssa_148, ssa_0
>      /* succs: block_12 block_37 */
> 
> compare to skipping the phi (19 NIR intructions)
> 
>      block block_11:
>      /* preds: block_9 block_10 */
>      vec1 32 ssa_125 = phi block_9: ssa_124, block_10: ssa_101
>      vec1 32 ssa_126 = fadd -ssa_96, ssa_120
>      vec1 32 ssa_127 = b32csel ssa_103, ssa_120, ssa_126
>      vec1 32 ssa_128 = fadd -ssa_96, ssa_125
>      vec1 32 ssa_129 = b32csel ssa_107, ssa_125, ssa_128
>      vec1 32 ssa_130 = ffma -ssa_78, ssa_6, ssa_104
>      vec1 32 ssa_131 = fge32 abs(ssa_127), ssa_95
>      vec1 32 ssa_132 = b32csel ssa_131, ssa_104, ssa_130
>      vec1 32 ssa_133 = ffma -ssa_79, ssa_6, ssa_106
>      vec1 32 ssa_134 = b32csel ssa_131, ssa_106, ssa_133
>      vec1 32 ssa_135 = fge32 abs(ssa_129), ssa_95
>      vec1 32 ssa_136 = iand ssa_135, ssa_131
>      vec1 32 ssa_137 = b2i32 ssa_136
>      vec1 32 ssa_138 = ffma ssa_78, ssa_6, ssa_111
>      vec1 32 ssa_139 = b32csel ssa_135, ssa_111, ssa_138
>      vec1 32 ssa_140 = ffma ssa_79, ssa_6, ssa_113
>      vec1 32 ssa_141 = b32csel ssa_135, ssa_113, ssa_140
>      vec1 32 ssa_142 = inot -ssa_137
>      vec1 32 ssa_143 = ine32 ssa_142, ssa_0
>      /* succs: block_12 block_37 */
> 
> There could be other interactions preventing it to get rid of that I'm
> missing.  But I think a good approach here could be: if there's
> another source in the phi that is the same as the one being replaced,
> just skip it since it would prevent the phi being directly removed.
> 
> 
> 	Caio
>