[Mesa-dev] [PATCH 06/11] glsl: Refactor logic for determining whether to lower return statements.

[Kenneth Graunke]

On 07/07/2011 09:56 AM, Paul Berry wrote:
> On 7 July 2011 01:16, Kenneth Graunke <kenneth at whitecape.org> wrote:
>> What exactly is the difference between lowering returns in "main" and
>> lowering them in other subroutines?  void vs. non-void is definitely
>> different, but I don't see why main is special.  Looking at the code, it
>> doesn't actually seem to use lower_main_return anywhere...
> 
> There's no difference in how returns are lowered in "main" and in
> subroutines.  The difference is in whether or not we lower returns.
> For instance, when _mesa_ir_link_shader() calls do_lower_jumps(), it
> always asks it to lower returns in subroutines (presumably to
> facilitate inlining), but it only asks it to lower returns in main if
> options->EmitNoMainReturn is true.

You're right of course, sorry...I somehow missed that
lower_main_return/lower_sub_return were options to the lowering pass.

I didn't realize that lower_jumps was needed for inlining (though it
sure makes sense).  opt_function_inlining.cpp has its own code for
this---the "replace_return_with_assignment" function.  Looking at that
again, it's fairly limited...it doesn't do any guarding in the case of
conditional returns...so it's really only useful for replacing the
canonical jump at the end of a non-void function.

Yet, we're using visit_tree with replace_return_with_assignment.  I
believe that this means we're actually walking over the whole IR tree
for the function (including all the expression trees!) looking for
returns.  That seems wasteful---if we're using lower_jumps, we know
exactly where it is...it's the last instruction.

Furthermore, I think can_inline probably ought to be altered.  Right now
it returns true if there's only one "return".  But it doesn't seem to be
checking that it's the _canonical_ return.

Without lower_jumps, I think the following code would be incorrectly
inlined:

uniform bool b;
float foo() {
    if (b)
        return 0.0;
    gl_FragColor = vec4(1, 0, 0, 1);
    return 1.0;
}

IOW, opt_function_inlining has broken code, but we're not seeing any
ill-effects because we run lower_jumps first, which fixes things.  We
should probably make that dependency explicitly clear.