[Mesa-dev] [PATCH] Fix strict-aliasing violations in GLSL shader list implementation

[Matt Turner]

On Wed, Jun 24, 2015 at 2:05 PM, Davin McCall <davmac at davmac.org> wrote:
> Hi - I'm new here.

Welcome!

> I've recently started poking the Mesa codebase for little reason other than
> personal interest. In the "help wanted" section of the website it mentions
> aliasing violations as a target for newcomers to fix, so with that in mind
> I've attached a patch (against git head) which resolves a few of them, by
> targeting the linked list implementation (list.h) used in the GLSL
> compiler/optimizers. This change slightly increases the storage requirements
> for a list (adds one word) but resolves the blatant aliasing violation that
> was caused by the trick used to conserve that word in the first place.
>
> (I toyed with another approach - using a single sentinel node for both the
> head and tail of a list - but this was much more invasive, and meant that
> you could no longer check whether a particular node was a sentinel node
> unless you had a reference to the list, so I gave up and went with this
> simpler approach).
>
> The most essential change is in the 'exec_list' structure. Three fields
> 'head', 'tail' and 'tail_pred' are removed, and two separate sentinel nodes
> are inserted in their place. The old 'head' is replaced by
> 'head_sentinel.next', 'tail_pred' by 'tail_sentinel.prev', and tail (always
> NULL) by 'head_sentinel.prev' and 'tail_sentinel.next' (both always NULL).
>
> With this patch, I can build a working (though perhaps not 100% bug-free)
> Mesa without using -fno-strict-aliasing during compilation. Before the
> patch, applications using Mesa would hang at runtime.

Thanks for the patch. I'm impressed that fixing exec_list/exec_node
allows the removal of -fno-strict-aliasing (at least, we don't know of
the rest of the bugs yet :).

To summarize your change, instead of the head/tail/tail_pred pointers
with "tail" shared between two aliased exec_nodes, you've simply
allocated two sentinel exec_nodes inside exec_list directly.

Like Ilia says, some memory usage statistics before/after your patch
would be very welcome. There's an apitrace of Dota2 that we've often
used for memory usage testing (via valgrind massif):

http://people.freedesktop.org/~anholt/dota_linux.trace

> I don't really know the process you have in place so if I need to do
> anything else to get this patch into the codebase please let me know. Any
> comments and criticisms welcome.

Please send patches inline using git send-email with a commit title
and message that looks similar to others. In this case, the patch
should be titled something like "glsl: Modify exec_list to avoid
strict-aliasing violations."

If you wanted to go farther and remove -fno-strict-aliasing from configure.ac --

Since strict-aliasing allows some additional optimization
opportunities, I'd expect some justification for the patch such as the
output of the 'size' command on the resulting binaries, like in this
commit:

commit d35720da9b9824d104532028775e497491f433ad
Author: Matt Turner <mattst88 at gmail.com>
Date:   Wed Mar 4 17:27:21 2015 -0800

    i965: Mark paths in linear <-> tiled functions as unreachable().

    text    data     bss     dec     hex filename
    9663       0       0    9663    25bf intel_tiled_memcpy.o   before
    8215       0       0    8215    2017 intel_tiled_memcpy.o   after

Presumably if strict-aliasing indeed allows additional optimizations,
we'd see smaller .text sizes after the removal of
-fno-strict-aliasing.