[Mesa-dev] [PATCH 2/4] glsl: Emit function signatures at toplevel, even for built-ins.

[Paul Berry]

On 1 August 2011 18:08, Ian Romanick <idr at freedesktop.org> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 08/01/2011 04:07 PM, Paul Berry wrote:
>> The ast-to-hir conversion needs to emit function signatures in two
>> circumstances: when a function declaration (or definition) is
>> encountered, and when a built-in function is encountered.
>>
>> To avoid emitting a function signature in an illegal place (such as
>> inside a function), emit_function() checked whether we were inside a
>> function definition, and if so, emitted the signature before the
>> function definition.
>>
>> However, this didn't cover the case of emitting function signatures
>> for built-in functions when those built-in functions are called from
>> global scope (e.g. a built-in function called from inside the constant
>> integer expression that specifies the length of an array).
>>
>> This patch changes emit_function() so that it emits function
>> signatures at toplevel in all cases.
>
> There's something about this patch that I don't grok.  The
> state->current_function test in emit function exists specifically to
> handle calls to functions (built-in or otherwise) at global scope.
> Without that, code such as the snippet below would not work, and text in
> the commit log seems to indicate that it shouldn't work.  However, I
> verified that it does.
>
> #version 120
> const float x = cos(3.14159 / 2.0);

I'm confused.  Do you mean to say that you didn't expect the above
code to work before applying this patch, or you didn't expect the
above code to work after applying this patch?

Before applying this patch, the above code works because when
processing a global initializer, we are emitting IR at toplevel scope.
 Since state->current_function is NULL, emit_function() just emits the
declaration in place, which is fine because function declarations are
allowed at toplevel scope.

After applying this patch, the above code still works because
emit_function() always emits the declaration at toplevel scope.

The case this patch is designed to fix is when a built-in function is
called from inside the constant integer expression that specifies the
length of an array, e.g.:

#version 120
float x[cos(3.14159/2.0) < 0.5 ? 1 : 2];

This case fails before the patch, because when processing an array
length, we are emitting IR into a dummy exec_list (see
process_array_type() in ast_to_hir.cpp).  process_array_type() later
checks (via an assertion) that no instructions were emitted to the
dummy exec_list, based on the reasonable assumption that we shouldn't
need to emit instructions to calculate the value of a constant.  This
is the crux of bug 38625, which is the bug I'm trying to fix in this
patch series.

After applying this patch, the situation is improved, but the bug
still isn't fixed.  In addition to this change, we need to avoid
emitting a function call when constant-folding a builtin function.
That's the subject of the next patch in the series.  I should have
made this clearer in the commit message.  I'll update it to clarify.