Re: Problem with stores and loads from unions and proposed fix

[Richard Guenther <richard dot guenther at gmail dot com>]

On Sat, Feb 6, 2010 at 3:44 AM, Martin Chaney <chaney@xkl.com> wrote:
> This problem showed up in a PDP10 C version of GCC I'm responsible for and
> took a good while to track down. ?The fix is in generic gcc code so even
> though my PDP10 compiler is not an official gcc version and I haven't been
> successful at creating a failing program on the Intel compiler it seems like
> it should cause problems elsewhere so I figured I should pass it on.
>
> Here's a union that allows referencing bits in a word in different ways (the
> PDP10 has a 36 bit word, but that doesn't seem to be an issue here)
>
> ? union {
> ? ? ? int word;
> ? ? ? struct {
> ? ? ? ? ? unsigned long w0 : 32;
> ? ? ? ? ? unsigned long pad : 4;
> ? ? ? } i32;
> ? ? ? struct {
> ? ? ? ? ? unsigned long s0 : 16;
> ? ? ? ? ? unsigned long s1 : 16;
> ? ? ? ? ? unsigned long pad : 4;
> ? ? ? } i16;
> ? ? ? struct {
> ? ? ? ? ? unsigned long b0 : 8;
> ? ? ? ? ? unsigned long b1 : 8;
> ? ? ? ? ? unsigned long b2 : 8;
> ? ? ? ? ? unsigned long b3 : 8;
> ? ? ? ? ? unsigned long pad : 4;
> ? ? ? } i8;
> ? } u32;
>
>
> u32.word = .... ;
>
> /* in a subsequent different basic block which is guaranteed to be reached
> with u32 unchanged */
> u32.i8.b1 = ... ;
> ... = u32.word ;
>
> CSE detects that the same subexpression is used in two places and
> substitutes a reaching register for the reference to u32.word without
> noticing that the memory has been modified by the bit field reference.
> ?Adding a call to invalidate_any_buried_refs(dest) flags the memory
> reference in such a way that it is not ignored and the erroneous CSE
> optimization is not done.
>
>
> --- gcse.c ? ? ?(revision 156482)
> +++ gcse.c ? ? ?(working copy)
> @@ -4783,7 +4783,12 @@ compute_ld_motion_mems (void)
> ? ? ? ? ? ? ? ? ? ? else
> ? ? ? ? ? ? ? ? ? ? ? ptr->invalid = 1;
> ? ? ? ? ? ? ? ? ? }
> - ? ? ? ? ? ? ? }
> + ? ? ? ? ?else
> + ? ? ? ? ? ?{
> + ? ? ? ? ? ? ?/* Make sure there isn't a buried store somewhere. ?*/
> + ? ? ? ? ? ? ?invalidate_any_buried_refs (dest);
> + ? ? ? ? ? ?}
> + ? ? ? }
> ? ? ? ? ? ? else
> ? ? ? ? ? ? ? invalidate_any_buried_refs (PATTERN (insn));
> ? ? ? ? ? }
>
>
> Thanks to anyone who can help determine whether this is a problem for other
> gcc versions and getting a fix into the gcc source.

Please specify the GCC version this happens with and show the RTL
before gcse.

Richard.

> Martin Chaney
> XKL, LLC
>
>
>
>
>
>
>