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

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/02/2011 12:18 PM, Paul Berry wrote:
> 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?

I'm saying that it does work, and it is supposed to work.  Your commit
message implies that it does not work before the 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

This is the bit of text that should be in the commit message. :)  Now it
makes sense.  The bit about the dummy exec_list is the important part...
that people familiar with the code may have forgotten.

> 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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk44nhsACgkQX1gOwKyEAw/l6ACfaBeNcJLpoWi1ZDthKrL46OHa
Re8AnRGplb1Pz0D2dN26CKTcMYz+3OR6
=c4uK
-----END PGP SIGNATURE-----