On Wed, 2019-01-02 at 10:16 -0600, Jason Ekstrand wrote: > On Wed, Jan 2, 2019 at 9:43 AM Ilia Mirkin <imir...@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 <yuq...@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
