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

[Iago Toral]

On Mon, 2015-11-09 at 12:27 -0500, Connor Abbott wrote:
> 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.

Ok, sounds reasonable to me. I'll make vecN opcodes unsigned again and
add asserts in the driver to catch 64-bit integer ALU operations.

Thank you both for the suggestions!

Iago

> >
> > 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
>