[Mesa-dev] glsl: Improvements to lower_jumps.cpp

[Ian Romanick]

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

On 07/05/2011 03:07 PM, Paul Berry wrote:
> The following patch series fixes bug #36669 (EmitNoMainReturn set
> to 1 doesn't make the GLSL compiler lower all the RET opcodes) as
> well as several other bugs I found in the course of reviewing
> lower_jumps.cpp.  Some of these bugs prevented certain jumps from
> ever being lowered, or produced assertion failures.  Others
> merely caused sub-optimal GPU code to be generated, by forcing
> do_lower_jumps() to take multiple passes in order to lower all
> jumps.
> 
> The optimization performed by lower_jumps.cpp is quite complex
> and has a number of subtle corner cases.  In order to be
> confident that these changes were correct, I added unit tests
> that verify its operation in isolation.  I'm particularly
> interested to hear comments on how the tests are structured.  I'm
> not aware of unit tests having been used in the mesa project
> before, and I'd like to find a way that we can make unit testing
> easier to do in other parts of the project (e.g. other
> optimization passes).

The only other unit tests in Mesa are for the old matrix math routines
(fixed function).  See src/mesa/math/m_debug*.c.

This is an area that I've been thinking about lately.  I noticed that
XCB uses a framework called check (http://check.sourceforge.net/), and
I've been wanting to talk to Jamey and Josh about their experience with
it, but I never seem to get around to it.  Maybe now is the right time. :)

> The unit tests are written in Python, and may be found in
> src/glsl/testing.  They make use of a new stand-alone executable,
> glsl_test, to invoke the jump lowering optimization in isolation.
> They may be run using "make check".
> 
> The unit tests, and the glsl_test executable they rely on, are
> build-time artifacts only.  They do not affect what is installed
> on the user's system by "make install".
> 
> [PATCH 01/11] glsl: Remove unused function prototypes.
> [PATCH 02/11] glsl: Make ir_reader able to read plain (return) statements.

These both look fine as is.

> [PATCH 03/11] glsl: Create a standalone executable for testing optimization passes.
> [PATCH 04/11] glsl: Add a unit test for lower_jumps.cpp

I'll send replies to these patches.

> [PATCH 05/11] glsl: Add explanatory comments to lower_jumps.cpp.

Wow.  This looks great as is.

> [PATCH 06/11] glsl: Refactor logic for determining whether to lower return statements.
> [PATCH 07/11] glsl: Lower unconditional return statements.

These both look fine as is.

> [PATCH 08/11] glsl: lower unconditional returns and continues in loops.

I'll send a reply to this patch.

> [PATCH 09/11] glsl: Use foreach_list in lower_jumps.cpp
> [PATCH 10/11] glsl: In lower_jumps.cpp, lower both branches of a conditional.
> [PATCH 11/11] glsl: Lower break instructions when necessary at the end of a loop.

I need to think about these a bit more.  It seems like the first change
(09/11) should allow the other changes to work, but it still makes me
nervous.  All of these passes track some aspects of the CFG and do
damage to it.  My concern is that assumptions about the state of the CFG
may not hold after it gets modified.

I don't really care if this means that we have to run multiple passes to
get them all.  We don't have any indication that this is causing a
particular CPU bottleneck.

One other question: does this series cause any piglit regressions on
platforms that require flow-control lowering (e.g., i915 or r300)?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEUEARECAAYFAk4UtPoACgkQX1gOwKyEAw+oxgCXcE+VYvFB+by/6soFx4GrBhkJ
WQCfaDb25IR+dkpj50BEezf8etDa6iQ=
=SJB8
-----END PGP SIGNATURE-----