[Mesa-dev] [PATCH] nir: Make vec-to-movs handle src/dest aliasing.

[Connor Abbott]

On Mon, Jan 26, 2015 at 2:31 PM, Eric Anholt <eric at anholt.net> wrote:
> Connor Abbott <cwabbott0 at gmail.com> writes:
>
>> On Fri, Jan 23, 2015 at 5:34 PM, Eric Anholt <eric at anholt.net> wrote:
>>> Connor Abbott <cwabbott0 at gmail.com> writes:
>>>
>>>> Argh, nevermind, I was reading it wrong...
>>>>
>>>> On Thu, Jan 22, 2015 at 8:18 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>>>>> What happens if you have something like foo = vec3(foo.z, bar.x,
>>>>> foo.x)? I don't think emitting vector mov's for only the contiguous
>>>>> components is enough.
>>>>>
>>>>> On Thu, Jan 22, 2015 at 4:51 PM, Eric Anholt <eric at anholt.net> wrote:
>>>>>> +static unsigned
>>>>>> +insert_movs(nir_alu_instr *vec, unsigned start_channel,
>>>>>> +            unsigned start_src_idx, void *mem_ctx)
>>>>
>>>> We need a comment explaining what this function does and what it
>>>> returns. Also, it only creates a single move so it should be called
>>>> insert_mov().
>>>
>>> How about:
>>>
>>> /**
>>>  * For a given writemask channel in the vec instruction, insert a MOV of all
>>>  * the src values that come from the same reg to the destination of the vec
>>>  * instruction.
>>>  */
>>
>> First, I think we need to be a little more clear on what it does, e.g.
>> "for a given starting channel/source, scans the rest of the sources of
>> the vec instruction to find all sources that come from the same reg
>> and emits a writemasked MOV from the reg to the destination of the vec
>> instruction." Also, you need to explain what the return value is (a
>> mask of all the channels/sources we inserted a MOV for).
>
> Maybe:
>
> /**
>  * For a given starting writemask channel and corresponding source index in
>  * the vec instruction, insert a MOV to the vec instruction's dest of all the
>  * writemask channels that get read from the same src reg.
>  *
>  * Returns the writemask of our MOV, so the parent loop calling this knows
>  * which ones have been processed.
>  */

Sounds good. After inserting this comment and renaming the function to
insert_mov(), this is

Reviewed-by: Connor Abbott <cwabbott0 at gmail.com>