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

[Paul Berry]

On 7 July 2011 10:53, Kenneth Graunke <kenneth at whitecape.org> wrote:
> 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.
>

That's a good point.  I'll make a note to have a look at that when I'm
finished working on lower_jumps.