mat4: m[i][j] incorrect result with row_major UBO & SSBO

Submitted by flo..@..il.com

Assigned to mes..@..op.org

Description

Created attachment 136630 Failing piglit test (when test_variant = 2) for this bug.

I hit another bug while trying to get Banshee 3D https://github.com/BearishSun/BansheeEngine to work correctly on Mesa radeonsi (HD 7870) / amdgpu kernel module. I use git commit 2719467e right now, which is a few days old.

Accessing a "mat4 m" component, e.g. m[1][2], returns unexpected results if m is declared inside a UBO block and uses row_major matrix format. col_major works as expected. m[1].z also works. Some experiments indicate that for m[i][j], the UBO buffer is accessed at offset (i+j)4 instead of (i+j4)*4.

The invalid offsets for loading floats from the UBO are visible in the Mesa IR when linker.cpp is done (probably introduced by lower_ubo_reference()).

While exploring this issue, I prepared a piglit test case that tests for this. It fails on my setup (when test_variant = 2, the other tests succeed). I will attach it to this bug report and send it to the piglit list for review.

Possible Fix:

I doubt this is enough/correct, because I haven't fully grasped the IR processing in the glsl compiler and which fields/data types can/can't be row_major and the implications, but this simple change in lower_buffer_access.cpp fixes this test and apparently causes no piglit regressions, so it's probably a good place to start (line 309):

if (deref_array->array->type->is_vector()) { /* We get this when storing or loading a component out of a vector * with a non-constant index. This happens for v[i] = f where v is * a vector (or m[i][j] = f where m is a matrix). If we don't * lower that here, it gets turned into v = vector_insert(v, i, * f), which loads the entire vector, modifies one component and * then write the entire thing back. That breaks if another * thread or SIMD channel is modifying the same vector. */ //array_stride = 4; // git master array_stride = *row_major ? 16 : 4; // "fixed" line if (deref_array->array->type->is_64bit()) array_stride *= 2;

~~Attachment 136630~~, "Failing piglit test (when test_variant = 2) for this bug.":
mat4-row-major-column-major-deref.shader_test

Version: git

To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information