[Mesa-dev] nir/i965: Source modifiers on vecN opcodes

[Connor Abbott]

On Mon, Nov 9, 2015 at 10:41 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>
> On Nov 9, 2015 7:24 AM, "Connor Abbott" <cwabbott0 at gmail.com> wrote:
>>
>> On Mon, Nov 9, 2015 at 6:55 AM, Iago Toral <itoral at igalia.com> wrote:
>> > Hi,
>> >
>> > Currently, NIR defines vecN operations as unsigned (integer). The fp64
>> > patches from Connor change this to float (I guess because we need to
>> > know the case where we are packing vectors of 64-bit floats). However,
>> > this makes it so that  nir_lower_source_to_mods turns this:
>> >
>> >         vec1 ssa_2 = fmov -ssa_1.y
>> >         vec3 ssa_3 = vec3 ssa_1, ssa_2, ssa_0
>> >
>> > into:
>> >
>> >         vec3 ssa_2 = vec3 ssa_1, -ssa_1.y, ssa_0
>> >
>> > This only happens because the vec3 operation is defined as a float
>> > operation now, otherwise it would not try to do this. It is not clear to
>> > me if this is by design, I mean, have this kind of things only kick-in
>> > for float/int and define vecN operations as unsigned to avoid this for
>> > them.
>> >
>> > The problem comes later when we call nir_lower_vec_to_movs in the i965
>> > vec4 backend. That pass generates a separate MOV for each component in
>> > the vector, but to do that properly when a negate is involved it needs
>> > to know if this is a float or an integer operand, which it  does not
>> > know at this point. The current code always emits an imov, which won't
>> > work if the operand is a float.
>> >
>> > I can think of two solutions for this:
>> >
>> > 1) Change nir_lower_source_to_mods so it does not try to rewrite alu
>> > operations where a source comes from a fmov with a negate, or at least
>> > if the instruction we are trying to rewrite is a vecN operation (or
>> > maybe allow this in scalar mode only?)
>> >
>> > 2) In nir_lower_vec_to_movs, if a source is negated, check its
>> > parent_instr and try to guess its type from that (in this example, we
>> > would see it came from fmov and we can say it is a float and emit fmov
>> > instead of imov). Not sure if this would work in all possible scenarios
>> > though.
>> >
>> > Opinions?
>> >
>> > Iago
>> >
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>> The only reason I changed vecN to produce floats is to avoid producing
>> 64-bit integer instructions, which at one point the constant folding
>> infrastructure couldn't support (but now it can), so you can just
>> revert the change. Ofc the i965 backend won't be able to express this
>> directly, but for now you can silently change 64-bit integers to
>> floats and assert that they only happen in things that copy data
>> around.
>
> I would tend to agree.  We could also make it unsigned so no source
> modifiers ever make sense.  Meh.

Oh yeah, I meant assert that we don't get e.g. a 64-bit iadd, so we
remember to fix that later. When we get support for real 64-bit
integers, we'll have to only map nir_type_int64/uint64 to DF on gen7.

>
> If we did want to keep vecN float, the thing to do would be to make
> vec_to_move lower it to fmovs rather than imovs.  But, like Connor said,
> just asserting no source modifiers for th 64-bit version in the backend is
> probably best.
>
> --Jason