[Bug target/52394] New: SH Target: SH2A defunct bitops

[Bug target/52394] New: SH Target: SH2A defunct bitops

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52394

             Bug #: 52394
           Summary: SH Target: SH2A defunct bitops
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: olegendo@gcc.gnu.org
                CC: kkojima@gcc.gnu.org
            Target: sh2a-*-*


As of rev 184582 the following SH2A bitops tests are failing:

FAIL: gcc.target/sh/sh2a-band.c scan-assembler band.b
FAIL: gcc.target/sh/sh2a-bld.c scan-assembler bld.b
FAIL: gcc.target/sh/sh2a-bor.c scan-assembler bor.b
FAIL: gcc.target/sh/sh2a-bxor.c scan-assembler bxor.b

It seems that the corresponding insns are not generated.

[Bug target/52394] SH Target: SH2A defunct bitops

[kkojima at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52394

--- Comment #1 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-02-28 00:37:56 UTC ---
I guess that now these tests require -fno-strict-volatile-bitfields,
though it isn't enough to avoid failures.  It looks that something
wrong happens in expmed.c:{store, extract}_bit_field_1 and they decide
to use slow fallback {store, extract}_fixed_bit_field instead of
generating insv/extv.

Here is suspicious part of {store, extract}_bit_field_1:

      /* Now convert from counting within UNIT to counting in EXT_MODE.  */
      if (BYTES_BIG_ENDIAN && !MEM_P (xop0))
    xbitpos += GET_MODE_BITSIZE (ext_mode) - unit;

      unit = GET_MODE_BITSIZE (ext_mode);

      /* If BITS_BIG_ENDIAN is zero on a BYTES_BIG_ENDIAN machine, we count
         "backwards" from the size of the unit we are extracting from.
     Otherwise, we count bits from the most significant on a
     BYTES/BITS_BIG_ENDIAN machine.  */

      if (BITS_BIG_ENDIAN != BYTES_BIG_ENDIAN)
    xbitpos = unit - bitsize - xbitpos;

In the problematic cases, xop0 is a QImode memory and ext_mode is SImode.
The initial value of unit is 8.  When starting xbitops is 3 and bitsize is
1 for example, these lines set xbitspos to 28!  There is no insv/extv which
inserts/extracts such bit position for QImode memory and maybe_expand_insn
for CODE_FOR_{insv, extv} fails.  Perhaps, these parts should be something
like

      /* We have been counting XBITPOS within UNIT.
     Count instead within the size of the register.  */
      if (BYTES_BIG_ENDIAN && !MEM_P (xop0))
    xbitpos += GET_MODE_BITSIZE (op_mode) - unit;

      /* If BITS_BIG_ENDIAN is zero on a BYTES_BIG_ENDIAN machine, we count
         "backwards" from the size of the unit we are inserting into.
     Otherwise, we count bits from the most significant on a
     BYTES/BITS_BIG_ENDIAN machine.  */

      if (BITS_BIG_ENDIAN != BYTES_BIG_ENDIAN)
    {
      if (!MEM_P (xop0))
        xbitpos = GET_MODE_BITSIZE (op_mode) - bitsize - xbitpos;
      else
        xbitpos = unit - bitsize - xbitpos;
    }

      unit = GET_MODE_BITSIZE (op_mode);

though I don't understand these routines well.

[Bug target/52394] SH Target: SH2A defunct bitops

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52394

--- Comment #2 from Oleg Endo <olegendo at gcc dot gnu.org> ---
The SH2A bitops seem to produce some of the insns, but it seems the generated
code is either really bad (defeating the original purpose) or broken.

For example:


volatile struct
{
  union
  {
    unsigned char BYTE;
    struct
    {
      unsigned char BIT7:1;
      unsigned char BIT6:1;
      unsigned char BIT5:1;
      unsigned char BIT4:1;
      unsigned char BIT3:1;
      unsigned char BIT2:1;
      unsigned char BIT1:1;
      unsigned char BIT0:1;
    }
    BIT;
  }
  ICR0;
}
USRSTR;

int
main (unsigned char a, unsigned char b, unsigned char c)
{
  USRSTR.ICR0.BIT.BIT5 |= b & 1;
  return 0;
}

compiled with -m2a -mb -mbitops -O2:
        mov.l   .L2,r2        // addr of USRSTR

        mov     #1,r1
        and     r1,r5         // b & 1

        mov     #0,r0
        bor.b   #5,@(0,r2)    // T |= (bit in mem)

        movt    r7            // r7 = T
        mov.b   @r2,r3        // load byte
        cmp/pl  r7            // T = r7
        mov     #5,r7
        movt    r1            // r1 = T
        bclr    #5,r3         // clear bit 5 of loaded byte
        shld    r7,r1         // T << 5
        or      r3,r1         // OR bit
        mov.b   r1,@r2        // write back
        rts/n

It seems that this will produce funny results at the first bor.b insn because
at function entry the T bit is undefined.

The code should actually be something like this:
        mov.l   .L2,r2
        bld     #0,r5
        mov     #0,r0
        bor.b   #5,@(0,r2)
        bst.b   #5,@(0,r2)

[Bug target/52394] SH Target: SH2A defunct bitops

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52394

--- Comment #3 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Oleg Endo from comment #2)
> 
> The code should actually be something like this:
>         mov.l   .L2,r2
>         bld     #0,r5
>         mov     #0,r0
>         bor.b   #5,@(0,r2)
>         bst.b   #5,@(0,r2)

Actually this might also result in unexpected side-effects when accessing
external hardware, because the volatile mem is accessed with 2x load and 1x
store.  So actually, this sequence can't be really used safely.

Moreover, it seems the code size improvements for those SH2A bitops are not so
big.  The above code is 14 bytes.  The same on non-SH2A could be:

        shlr    r5
        subc    r0,r0
        not     r0,r0
        and     #32,r0
        mov.l   .L5,r2
        mov.b   @r2,r1
        or      r0,r1
        mov.b   r1,@r2

which is 16 bytes.

And the SH2A version of that could be:
        bld     #0,r5
        mov     #0,r0
        bst     #5,r0
        mov.l   .L5,r2
        mov.b   @r2,r1
        or      r0,r1
        mov.b   r1,@r2

which is 14 bytes.

And of course, if GBR can be clobbered it gets down to 8 bytes:
        mov.l   .L5,r2
        ldc     r2,gbr
        mov     #0,r0
        or.b    #32,@(r0,gbr)