[Lima] [Mesa-dev] NIR constant problem for GPU which doesn't have native integer support

Thu Jan 3 09:39:25 UTC 2019

On Wed, 2019-01-02 at 10:16 -0600, Jason Ekstrand wrote:
> On Wed, Jan 2, 2019 at 9:43 AM Ilia Mirkin <imirkin at alum.mit.edu>
> wrote:
> > Have a look at the first 4 patches in the series from Jonathan
> > Marek
> > to address some of these issues:
> > 
> > https://patchwork.freedesktop.org/series/54295/
> > 
> > Not sure exactly what state that work is in, but I've added
> > Jonathan
> > to CC, perhaps he can provide an update.
> > 
> > Cheers,
> > 
> >   -ilia
> > 
> > On Wed, Jan 2, 2019 at 6:28 AM Qiang Yu <yuq825 at gmail.com> wrote:
> > >
> > > Hi guys,
> > >
> > > I found the problem with this test fragment shader when lima
> > development:
> > > uniform int color;
> > > void main() {
> > >     if (color > 1)
> > >         gl_FragColor = vec4(1.0, 0.0, 0.0, 1);
> > >     else
> > >         gl_FragColor = vec4(0.0, 1.0, 0.0, 1);
> > > }
> > >
> > > nir_print_shader output:
> > > impl main {
> > >         block block_0:
> > >         /* preds: */
> > >         vec1 32 ssa_0 = load_const (0x00000001 /* 0.000000 */)
> > >         vec4 32 ssa_1 = load_const (0x3f800000 /* 1.000000 */,
> > > 0x00000000 /* 0.000000 */, 0x00000000 /* 0.000000 */, 0x3f800000
> > /*
> > > 1.000000 */)
> > >         vec4 32 ssa_2 = load_const (0x00000000 /* 0.000000 */,
> > > 0x3f800000 /* 1.000000 */, 0x00000000 /* 0.000000 */, 0x3f800000
> > /*
> > > 1.000000 */)
> > >         vec1 32 ssa_3 = load_const (0x00000000 /* 0.000000 */)
> > >         vec1 32 ssa_4 = intrinsic load_uniform (ssa_3) (0, 1, 0)
> > /*
> > > base=0 */ /* range=1 */ /* component=0 */   /* color */
> > >         vec1 32 ssa_5 = slt ssa_0, ssa_4
> > >         vec1 32 ssa_6 = fnot ssa_5
> > >         vec4 32 ssa_7 = bcsel ssa_6.xxxx, ssa_2, ssa_1
> > >         intrinsic store_output (ssa_7, ssa_3) (0, 15, 0) /*
> > base=0 */
> > > /* wrmask=xyzw */ /* component=0 */       /* gl_FragColor */
> > >         /* succs: block_1 */
> > >         block block_1:
> > > }
> > >
> > > ssa0 is not converted to float when glsl to nir. I see
> > glsl_to_nir.cpp
> > > will create flt/ilt/ult
> > > based on source type for gpu support native integer, but for gpu
> > not
> > > support native
> > > integer, just create slt for all source type. And in
> > > nir_lower_constant_initializers,
> > > there's also no type conversion for integer constant.
> 
> This is a generally sticky issue.  In NIR, we have no concept of
> types on SSA values which has proven perfectly reasonable and
> actually very powerful in a world where integers are supported
> natively.  Unfortunately, it causes significant problems for float-
> only architectures.

I would like to take this chance to say that this untyped SSA-value
choice has lead to issues in both radeon_si (because LLVM values are
typed) and zink (similarly, because SPIR-V values are typed), where we
need to to bitcasts on every access because there's just not enough
information available to emit variables with the right type.

It took us a lot of time to realize that the meta-data from the opcodes
doesn't *really* provide this, because the rest of nir doesn't treat
values consistently. In fact, this feels arguably more like buggy
behavior; why do we even have fmov when all of the time the compiler
will emit imovs for floating-point values...? Or why do we have bitcast

I would really love it if we could at least consider making this "we
can just treat floats as ints without bitcasts if we feel like it"-
design optional for the backend somehow.

I guess the assumption is that bitcasts are for free? They aren't once
you have to emit them and have a back-end remove a bunch of redundant
ones... We should already have all the information to trivially place
casts for backends that care, yet we currently make it hard (unless
your HW/backend happens to be untyped)...

Is there some way we can perhaps improve this for backends that care?

>   There are two general possible solutions:
> 
>  1. convert all integers to floats in glsl_to_nir and prog_to_nir and
> adjust various lowering/optimization passes to handle
> nir_compiler_options::supports_native_integers == false.
> 
>  2. Just allow integers all the way through until you get very close
> to the end and then lower integers to floats at the last possible
> moment.
> 
> Both of these come with significant issues.  With the first approach,
> there are potentially a lot of passes that will need to be adjusted
> and it's not 100% clear what to do with UBO offsets and indirect
> addressing; fortunately, you should be able to disable most of those
> optimizations to get going so it shouldn't be too bad.  The second
> would be less invasive to NIR because it doesn't require modifying as
> many passes.  However, doing such a lowering would be very tricky to
> get right primarily because of constants.  With everything else, you
> can just sort of assume that inputs are floats (you lowered, right?)
> and lower to produce a float output.  With constants, however, you
> don't know whether or not it's an integer that needs lowering.  We
> could, in theory, add an extra bit to load_const to solve this
> problem but there are also significant problems with doing that so
> it's not clear it's a good idea.
> 
> I think the patches from Marek (as indicated by ilia) attempt the
> first approach.  If we can do it practically, my suspicion is that
> the first will work better than the second.  However, it will take
> some experimentation in order to actually determine that.
> 
> --Jason
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev