Re: h8300: less optimal (buggy?) compiler output with last build

[Graham Stott <grahams at cygnus dot co dot uk>]

Ralf Gütlein wrote:
> 
> After installing the latest build of the h8 toolchain,
> I realized that the compiler output (assembler source)
> is worse than before (I used to use a snapshot from
> December 99). One concrete example (cc the use of inline
> functions):
> 
> extern unsigned char volatile TCW;
> 
> inline Watchdog()
> {
>   TCW = 0;
> }
> 
> void testcase_1(void)
> {
>   TCW = 0;
> }
> 
> void testcase_2(void)
> {
>   Watchdog();
> }
> 
> The h8 assembler source (compiled with -Os -fomit-frame-pointer):
> 
> testcase_1: sub   r2l, r2l
>             mov.b r2l, TCW
>             rts
> 
> testcase_2: sub   r2l, r2l
>             mov.b r2l, TCW
>             mov.b TCW, r2l  ; <-- ????
>             rts
> 
> As you can see, in the "inline" test case there is an erroneous read
> of the previously written address.
> This could be fatal (in case of an embedded system), when the
> hardware doesn't "like" read access to a write-only position. So there
> is good reason to treat this as a BUG.
> 
> As I mentioned before, this problem didn't occure in earlier versions
> (e.g. Dec-99).
> 
> Is this issue present in other ports?
> Will this be investigated by somebody?
> Or, could anybody give hints where to look for this issue in the sources?
> 
> Regards,
> Ralf

I took a look at this problem. It's target independent and the problem (
extranous read from volatile) will occur assignment to a volatile variable
within a GCC statement expression (i.e. ({...})). You are asking where is
the statement expression in the above code. Well it's added by the C++
front-end when inlining Watchdog.

Here's a C testcase which also shows the bug.
-----------------------------------------------------
extern volatile unsigned char TCW;

#define  WD()   \ 
({              \ 
  TCW = 0;      \
  1;            \
})

int test(void)
{
   return WD();
}
------------------------------------------------------

This is what I get compiling it with -O0 -S -g0 -fomit-frame-pointer
altough you get the same extra "movzbl TCW,%eax" no matter what options
are used.
 
------------------------------------------------------
        .file   "test.c"
        .version        "01.01"
gcc2_compiled.:
.text
        .align 16
.globl test
        .type    test,@function
test:
        movb    $0, TCW
        movzbl  TCW, %eax
        movl    $1, %eax
        ret
.Lfe1:
        .size    test,.Lfe1-test
        .ident  "GCC: (GNU) 2.96 20000817 (experimental)"

------------------------------------------------------

The extraneous "movzbl TCW,%eax" is coming from the copy_to_reg in
this block of code at the end of store_expr.

  /* If we don't want a value, return NULL_RTX.  */
  if (! want_value)
    return NULL_RTX;

  /* If we are supposed to return TEMP, do so as long as it isn't a MEM.
     ??? The latter test doesn't seem to make sense.  */
  else if (dont_return_target && GET_CODE (temp) != MEM)
    return temp;

  /* Return TARGET itself if it is a hard register.  */
  else if (want_value && GET_MODE (target) != BLKmode
           && ! (GET_CODE (target) == REG
                 && REGNO (target) < FIRST_PSEUDO_REGISTER))
    return copy_to_reg (target);

  else
    return target;
}

What I don't understand is why the copy_to_reg is needed! can't it just
return target? (thus)


  /* If we don't want a value, return NULL_RTX.  */
  if (! want_value)
    return NULL_RTX;

  /* If we are supposed to return TEMP, do so as long as it isn't a MEM.
     ??? The latter test doesn't seem to make sense.  */
  else if (dont_return_target && GET_CODE (temp) != MEM)
    return temp;
  else
    return target;
}


I built this hacked version and recompiled the testcase now gives
as expected.
------------------------------------------------------
        .file   "test.c"
        .version        "01.01"
gcc2_compiled.:
.text
        .align 16
.globl test
        .type    test,@function
test:
        movb    $0, TCW
        movl    $1, %eax
        ret
.Lfe1:
        .size    test,.Lfe1-test
        .ident  "GCC: (GNU) 2.96 20000817 (experimental)"

------------------------------------------------------

Comments?

Graham