cwabbott0 / lima_compiler

standalone GLSL ES compiler for lima
Other
9 stars 3 forks source link

Matrix multiplication problems #1

Open ssvb opened 10 years ago

ssvb commented 10 years ago

Division by zero trying to compile the following simple vertex shader:

    uniform mat4 modelviewprojectionMatrix;

    attribute vec4 in_position;  
    attribute vec2 in_coord;     

    varying vec2 coord;          

    void main()                  
    {                            
        gl_Position = modelviewprojectionMatrix * in_position;
        coord = in_coord;        
    }

==27873== Process terminating with default action of signal 8 (SIGFPE) ==27873== Integer divide by zero at address 0x4049E9641 ==27873== at 0x40346B: get_hash (ptrset.c:41) ==27873== by 0x40376D: ptrset_add (ptrset.c:114) ==27873== by 0x412DDF: lima_gp_ir_node_link (node.c:674) ==27873== by 0x454432: (anonymous namespace)::gp_ir_visitor::emit_uniform_load(irdereference, bool) (from_glsl.cpp:1222) ==27873== by 0x453CAC: (anonymous namespace)::gp_ir_visitor::handle_deref(irdereference) (from_glsl.cpp:1001) ==27873== by 0x453AC8: (anonymous namespace)::gp_ir_visitor::visit_enter(ir_dereferencearray) (from_glsl.cpp:950) ==27873== by 0x485436: ir_dereference_array::accept(ir_hierarchicalvisitor) (ir_hv_accept.cpp:272) ==27873== by 0x45368C: (anonymous namespace)::gp_ir_visitor::emit_expression(lima_gp_ir_op_e, ir_rvalue, unsigned int) (from_glsl.cpp:855) ==27873== by 0x452D42: (anonymous namespace)::gp_ir_visitor::visit_enter(irexpression) (from_glsl.cpp:616) ==27873== by 0x485249: ir_expression::accept(ir_hierarchicalvisitor) (ir_hv_accept.cpp:150) ==27873== by 0x45368C: (anonymous namespace)::gp_ir_visitor::emit_expression(lima_gp_ir_op_e, ir_rvalue, unsigned int) (from_glsl.cpp:855) ==27873== by 0x452D1F: (anonymous namespace)::gp_ir_visitor::visit_enter(ir_expression*) (from_glsl.cpp:612)

cwabbott0 commented 10 years ago

I'm not sure why you're getting a divide-by-zero error (on my machine, I get a segfault) but it seems that the underlying issue is that GLSL IR is lowering matrix operations like

modelviewprojectionMatrix * in_position

to

modelviewprojectionMatrix[0] * in_position.x + modelviewprojectionMatrix[1] * in_position.y + ...

which is good, except it seems to represent modelviewprojectionMatrix[0] as an ir_dereference_array, even though the thing being dereferenced isn't an array, which confuses us when we lower dereferences to offsets since we don't handle that case yet.

ssvb commented 10 years ago

Can this shader be modified into something that is currently supported?

cwabbott0 commented 10 years ago

Well, if you handcoded those operations instead of using matrices it should work... I'll try and fix the bug tonight though, so hopefully you won't have to. It's nice having people test this, even if the result is "oh whoops, I forgot to handle matrices!"

cwabbott0 commented 10 years ago

Since 6a240956fb5f7280e934552f91e1c6887460ba0b, we now generate correct GP IR for this shader, and valgrind gives me no errors, so this particular error shouldn't happen for you. However, for some reason, the scheduler keeps restarting without making any progress until it runs out of registers to be allocated the fast way, at which point it hits an assert since we haven't implemented the slow path (which involves generating driver-internal uniforms to hold constants, something we couldn't do until pretty recently). The IR before this seems sane, again valgrind shows no errors, and the scheduler should have no problem scheduling this, so there's one of 2 things happening:

  1. There's some subtle bug in the scheduler or earlier code that valgrind isn't catching.
  2. Our scheduling heuristics are somehow off and need some tweaking.

Either way, it's going to be a lot harder to debug... sorry.

ssvb commented 10 years ago

After trying to play around with the code and change mat4/vec4 to mat3/vec3, it segfaults again:

uniform mat3 modelviewprojectionMatrix;

attribute vec3 in_position;
attribute vec2 in_coord;

varying vec2 coord;

void main()
{
    vec3 tmp = modelviewprojectionMatrix * in_position;
    gl_Position = vec4(tmp.x, tmp.y, tmp.z, 1.0);
    coord = in_coord;
}
ssvb commented 10 years ago

Well, if you handcoded those operations instead of using matrices it should work... I'll try and fix > > the bug tonight though, so hopefully you won't have to. It's nice having people test this, even if the > result is "oh whoops, I forgot to handle matrices!"

FWIW, handcoding the matrix operation indeed works now at least in 51f162b1e25efddc5084782c530f503cdc843b08:

uniform mat4 modelviewprojectionMatrix;

attribute vec4 in_position;
attribute vec2 in_coord;

varying vec2 coord;

void main()
{
    gl_Position = vec4(modelviewprojectionMatrix[0][0] * in_position.x +
                       modelviewprojectionMatrix[1][0] * in_position.y +
                       modelviewprojectionMatrix[2][0] * in_position.z +
                       modelviewprojectionMatrix[3][0] * in_position.w,

                       modelviewprojectionMatrix[0][1] * in_position.x +
                       modelviewprojectionMatrix[1][1] * in_position.y +
                       modelviewprojectionMatrix[2][1] * in_position.z +
                       modelviewprojectionMatrix[3][1] * in_position.w,

                       modelviewprojectionMatrix[0][2] * in_position.x +
                       modelviewprojectionMatrix[1][2] * in_position.y +
                       modelviewprojectionMatrix[2][2] * in_position.z +
                       modelviewprojectionMatrix[3][2] * in_position.w,

                       modelviewprojectionMatrix[0][3] * in_position.x +
                       modelviewprojectionMatrix[1][3] * in_position.y +
                       modelviewprojectionMatrix[2][3] * in_position.z +
                       modelviewprojectionMatrix[3][3] * in_position.w);
    coord = in_coord;
}
cwabbott0 commented 10 years ago

Ok, that's strange... it must be that manually doing it reorders the adds/multiplies in such a way that makes us not hit that pathological case in the scheduler. Just for reference, you're now getting the same error as me when you don't hand-code it, right?

Assertion failed: (0), function try_schedule_reg, file gp_ir/scheduler.c, line 967.

ssvb commented 10 years ago

Just for reference, you're now getting the same error as me when you don't hand-code it, right? Assertion failed: (0), function try_schedule_reg, file gp_ir/scheduler.c, line 967.

limasc: gp_ir/scheduler.c:950: try_schedule_reg: Assertion `0' failed. Aborted