On Wed, Jun 24, 2015 at 2:05 PM, Davin McCall <dav...@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 <matts...@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. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
