[Bug rtl-optimization/50110] New: Endian reversal when adding extzv instruction

[Bug rtl-optimization/50110] New: Endian reversal when adding extzv instruction

[david.meggy at icron dot com]

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

             Bug #: 50110
           Summary: Endian reversal when adding extzv instruction
    Classification: Unclassified
           Product: gcc
           Version: 4.6.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: david.meggy@icron.com


We are in the process of adding a bitfield extract instruction to the SPARC
Leon processor for a custom embedded product

I've added the following to the sparc.md file

(define_insn "extzvsi"
  [(set (match_operand:SI 0 "register_operand" "=r")
       (zero_extract:SI (match_operand:SI 1 "register_operand" "r")
                         (match_operand 2 "const_int_operand" "")
                         (match_operand 3 "const_int_operand" "")))]
  ""
  "ext\t%1,%2,%3,%0"
  )

and GCC will start producing assembly code that uses the new opcode


However on simple test programs the endian seems to be reversed

struct x {
    unsigned int a:4;
    unsigned int b:2;
    unsigned int c:3;
    unsigned int d:3;
    unsigned int e:20;
};

unsigned int a(struct x * arg)
{ return arg->a; }

unsigned int b(struct x * arg)
{ return arg->b; }

unsigned int c(struct x * arg)
{ return arg->c; }

unsigned int d(struct x * arg)
{ return arg->d; }

unsigned int e(struct x * arg)
{ return arg->e; }


compiled with -Os -S

results in

        .file   "davidm.c"
        .section        ".text"
        .align 4
        .global a
        .type   a, #function
        .proc   016
a:
        ld      [%o0], %o0
        jmp     %o7+8
         srl    %o0, 28, %o0
        .size   a, .-a
        .align 4
        .global b
        .type   b, #function
        .proc   016
b:
        ld      [%o0], %o0
        jmp     %o7+8
         ext    %o0,2,4,%o0
        .size   b, .-b
        .align 4
        .global c
        .type   c, #function
        .proc   016
c:
        ld      [%o0], %o0
        jmp     %o7+8
         ext    %o0,3,6,%o0
        .size   c, .-c
        .align 4
        .global d
        .type   d, #function
        .proc   016
d:
        ld      [%o0], %o0
        jmp     %o7+8
         ext    %o0,3,9,%o0
        .size   d, .-d
        .align 4
        .global e
        .type   e, #function
        .proc   016
e:
        ld      [%o0], %g1
        sethi   %hi(-1048576), %o0
        jmp     %o7+8
         andn   %g1, %o0, %o0
        .size   e, .-e
        .ident  "GCC: (GNU) 4.6.1"

Just looking at functions a() and b(), a() is using the bitfields in big endian
order as it should for SPARC, but b() seems to be using the bitfields in little
endian order.

[Bug rtl-optimization/50110] Endian reversal when adding extzv instruction

[david.meggy at icron dot com]

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

--- Comment #1 from David Meggy <david.meggy at icron dot com> 2011-08-17 19:18:21 UTC ---
When I compile with with -da to dump all the temporary files the endian
reversal seems to happen in the .179r.combine file.

The following block of code is in the .178r.dce file


(insn 6 3 7 2 (set (reg:SI 114)
        (mem/s:SI (reg/v/f:SI 111 [ arg ]) [0+0 S4 A32])) davidm.c:13 39
{*movsi_insn}
     (expr_list:REG_DEAD (reg/v/f:SI 111 [ arg ])
        (nil)))

(insn 7 6 9 2 (set (reg:SI 113)
        (lshiftrt:SI (reg:SI 114)
            (const_int 26 [0x1a]))) davidm.c:13 258 {lshrsi3}
     (expr_list:REG_DEAD (reg:SI 114)
        (nil)))

(insn 9 7 10 2 (set (reg:SI 116)
        (and:SI (reg:SI 113)
            (const_int 3 [0x3]))) davidm.c:13 166 {andsi3}
     (expr_list:REG_DEAD (reg:SI 113)
        (nil)))

(insn 10 9 15 2 (set (reg:SI 112)
        (zero_extend:SI (subreg:QI (reg:SI 116) 3))) davidm.c:13 91
{*zero_extendqisi2_insn}
     (expr_list:REG_DEAD (reg:SI 116)
        (nil)))

(insn 15 10 18 2 (set (reg/i:SI 24 %i0)
        (reg:SI 112)) davidm.c:13 39 {*movsi_insn}
     (expr_list:REG_DEAD (reg:SI 112)
        (nil)))

which contains a lshrsi3 and andsi3 (not being familar with GCC internals, I
presume this is right shift 26 followed by and of 0x3).  Which looks correct

In the .179r.combine file following snippet is present

(insn 6 3 7 2 (set (reg:SI 114)
        (mem/s:SI (reg:SI 24 %i0 [ arg ]) [0+0 S4 A32])) davidm.c:13 39
{*movsi_insn}
     (expr_list:REG_DEAD (reg:SI 24 %i0 [ arg ])
        (nil)))

(note 7 6 9 2 NOTE_INSN_DELETED)

(note 9 7 10 2 NOTE_INSN_DELETED)

(note 10 9 15 2 NOTE_INSN_DELETED)

(insn 15 10 18 2 (set (reg/i:SI 24 %i0)
        (zero_extract:SI (reg:SI 114)
            (const_int 2 [0x2])
            (const_int 4 [0x4]))) davidm.c:13 88 {extzvsi}
     (expr_list:REG_DEAD (reg:SI 114)
        (nil)))


Which appears that this pass in the compiler swapped the endian as the extzvsi
instruction is now using the constants 2 and 4, which would represent a little
endian bitfield structure.

[Bug rtl-optimization/50110] Endian reversal when adding extzv instruction

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> 2011-08-17 19:31:56 UTC ---
I think you have issue how zero_extract RTL works.

[Bug rtl-optimization/50110] Endian reversal when adding extzv instruction

[david.meggy at icron dot com]

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

--- Comment #3 from David Meggy <david.meggy at icron dot com> 2011-08-17 19:37:48 UTC ---
Andrew, are you referring to an issue with the define_insn macro I created? or
the GCC zero-extract generic code?

I've taken a look at
http://gcc.gnu.org/onlinedocs/gccint/Standard-Names.html#Standard-Names
(scrolling down to extzv and extv), and I believe that I'm using the extraction
as intended to be used

[Bug rtl-optimization/50110] Endian reversal when adding extzv instruction

[david.meggy at icron dot com]

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

--- Comment #4 from David Meggy <david.meggy at icron dot com> 2011-08-17 23:15:26 UTC ---
>From rtl.def


/* Reference to a signed bit-field of specified size and position.
   Operand 0 is the memory unit (usually SImode or QImode) which
   contains the field's first bit.  Operand 1 is the width, in bits.
   Operand 2 is the number of bits in the memory unit before the
   first bit of this field.
   If BITS_BIG_ENDIAN is defined, the first bit is the msb and
   operand 2 counts from the msb of the memory unit.
   Otherwise, the first bit is the lsb and operand 2 counts from
   the lsb of the memory unit.
   This kind of expression can not appear as an lvalue in RTL.  */
DEF_RTL_EXPR(SIGN_EXTRACT, "sign_extract", "eee", RTX_BITFIELD_OPS)

/* Similar for unsigned bit-field.
   But note!  This kind of expression _can_ appear as an lvalue.  */
DEF_RTL_EXPR(ZERO_EXTRACT, "zero_extract", "eee", RTX_BITFIELD_OPS)



It appears that this behaviour is intended.  To me it seems logical that the
extract instruction is really just a "right shift", followed by an "and", where
the "and" uses a mask based on the width of the bitfield.  To count from the
MSB seems counter-intuitive to me.

If my understanding is correct this code comment should appear in
http://gcc.gnu.org/onlinedocs/gccint/Standard-Names.html#Standard-Names under
the extzv and extv headings.  I can fix up my define_insn to do the conversion
in our implementation.

[Bug rtl-optimization/50110] Endian reversal when adding extzv instruction

[david.meggy at icron dot com]

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

David Meggy <david.meggy at icron dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID

--- Comment #5 from David Meggy <david.meggy at icron dot com> 2012-01-10 19:16:15 UTC ---
Let me sum up this bug.  It boiled down to a misunderstanding of how bit field
extract works on a big endian processor (I don't know if anyone else has
implemented this on a big endian processor).  Since we have implemented the
starting bit in our hardware from the least significant bit, I've just modified
the output instruction to change the start bit to the least significant bit.

(define_insn "extzvsi"
  [(set (match_operand:SI 0 "register_operand" "=r")
       (zero_extract:SI (match_operand:SI 1 "register_operand" "r")
                         (match_operand 2 "const_int_operand" "")
                         (match_operand 3 "const_int_operand" "")))]
  ""
  "ext\t%1,%2,32 - %2 - %3,%0"
  )

This works quite well for us, and is giving us the code space savings we were
looking for, an ~1% code space reduction.  Not bad for a tiny GCC patch, tiny
binutils patch, and a minor CPU opcode addition.

This bug can be closed.  Marking as RESOLVED - INVALID, as it isn't a real bug,
but a documentation misunderstanding