On Mon, 2015-11-09 at 12:27 -0500, Connor Abbott wrote: > On Mon, Nov 9, 2015 at 10:41 AM, Jason Ekstrand <ja...@jlekstrand.net> wrote: > > > > On Nov 9, 2015 7:24 AM, "Connor Abbott" <cwabbo...@gmail.com> wrote: > >> > >> On Mon, Nov 9, 2015 at 6:55 AM, Iago Toral <ito...@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@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 > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev