[Mesa-dev] Follow up on potential inlining bug (was: [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:
> 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.
>

I just double-checked this, and it turns out that can_inline() _does_
check that the one return is the canonical return, but it does it in a
roundabout way: after counting the number of returns in the function,
it adds one if the function doesn't end in a return instruction (to
account for the "implicit return").  So if a function contains a
non-canonical return, it gets counted as having two returns, and the
function won't be inlined.  The bug you were worried about doesn't
exist.

Of course, failing to inline a function is bad too, but fortunately,
in all code paths, do_lower_jumps() is called before
opt_function_inlining(), so it's not an issue.  I'm not sure if that
was intentional or just a coincidence--the order of optimizations
seems a little ad-hoc :)