[Mesa-dev] [PATCH] Fix strict-aliasing violations in GLSL shader list implementation
mattst88 at gmail.com
Tue Jun 23 14:35:18 PDT 2015
On Wed, Jun 24, 2015 at 2:05 PM, Davin McCall <davmac at davmac.org> wrote:
> Hi - I'm new here.
> 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):
> 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
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
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
More information about the mesa-dev