[i386 PATCH] Improved bit-field assignment RTL expansion

[Roger Sayle <roger at eyesopen dot com>]

There are a number of bugs in bugzilla concerning the poor code
generated by recent versions of GCC for bit-field manipulations.
The patch below is the first in a series to improve the situation.

Consider the example program:

typedef struct {
  unsigned int bit1 : 1;
  unsigned int bit2 : 1;
  unsigned int bit3 : 1;
} T;

void foo(T *p, T *q)
{
  p->bit3 = q->bit3;
}

Currently with mainline at -O2 -fomit-frame-pointer we generate the
following on x86:

foo:    movl    8(%esp), %eax
        movl    4(%esp), %ecx
        movzbl  (%eax), %eax
        movl    (%ecx), %edx
        shrb    $2, %al
        andl    $1, %eax
        andl    $-5, %edx
        sall    $2, %eax
        orl     %eax, %edx
        movl    %edx, (%ecx)
        ret

You'll notice that this contains two shifts of different sizes,
neither of which gets optimized away by the RTL optimizers.  The
problem is that intial RTL expansion performs a read of a bitfield
in QImode, but a write to a bitfield in SImode.  This subtle
asymmetry is enough to cause problems, as combines three instruction
limit is too shallow to fix things up for us.

With the patch below, we now generate the nicer:

foo:    movl    8(%esp), %edx
        movl    4(%esp), %eax
        movzbl  (%edx), %edx
        andb    $-5, (%eax)
        andl    $4, %edx
        orb     %dl, (%eax)
        ret

Notice there are now no shifts.  The reason for the QImode/SImode
mismatch during RTL expansion is that the middle-end mistakenly
believes that it could use the target's INSV instruction to perform
the insertion, and so speculatively promotes the intermediates to
SImode to allow the native "insv" expander to be used. In the
case, above insv is inappropriate, so we fall back to shifts,
ands and iors, but now with modes that don't match for the read
vs. the write.

The work-around below is to allow the middle-end to be slightly more
cautious about when it attempts to generate code intended for use
with an INSV, by checking whether the bitsize operand would match the
backend's predicate in addition to current HAVE_insv.  This can
avoid the problems of incorrectly speculating that insv can be
expanded.  Then to take advantage of this, the i386 backend is
tweaked to define a new "const8_operand" which is used in its
insv, extv, and extzv expanders in place of the current less
strict "immediate_operand".

Eagle-eyed cycle counters can probably guess the next patch in this
series from the new code sequence we generate above.


The following patch has been tested on i686-pc-linux-gnu with a
full "make bootstrap", all default languages including Ada, and
regression tested with a top-level "make -k check" with no new
failures.  Ok for mainline?

I'll concede that this approach isn't ideal.  It would be nicer to
completely restructure store_bit_field to avoid the recursion and
allow RTL expansion to use the more usual generate-and-fail idiom.
But that's much more instrusive and probably best tackled when we
generalize insv to become a convert_optab.  This enhancement atleast
keeps in the spirit of the current design.



2006-04-25  Roger Sayle  <roger@eyesopen.com>

	* expmed.c (store_bit_field): Also check whether the bitsize is
	valid for the machine's "insv" instruction before moving the
	target into a pseudo for use with the insv.
	* config/i386/predicates.md (const8_operand): New predicate.
	* config/i386/i386.md (extv, extzv, insv): Use the new
	const8_operand predicate where appropriate.


Index: expmed.c
===================================================================
*** expmed.c	(revision 113016)
--- expmed.c	(working copy)
*************** store_bit_field (rtx str_rtx, unsigned H
*** 618,624 ****
        && bitsize > 0
        && GET_MODE_BITSIZE (op_mode) >= bitsize
        && ! ((REG_P (op0) || GET_CODE (op0) == SUBREG)
! 	    && (bitsize + bitpos > GET_MODE_BITSIZE (op_mode))))
      {
        int xbitpos = bitpos;
        rtx value1;
--- 618,626 ----
        && bitsize > 0
        && GET_MODE_BITSIZE (op_mode) >= bitsize
        && ! ((REG_P (op0) || GET_CODE (op0) == SUBREG)
! 	    && (bitsize + bitpos > GET_MODE_BITSIZE (op_mode)))
!       && insn_data[CODE_FOR_insv].operand[1].predicate (GEN_INT (bitsize),
! 							VOIDmode))
      {
        int xbitpos = bitpos;
        rtx value1;
Index: config/i386/predicates.md
===================================================================
*** config/i386/predicates.md	(revision 113016)
--- config/i386/predicates.md	(working copy)
***************
*** 544,549 ****
--- 544,554 ----
    (and (match_code "const_int")
         (match_test "op == const1_rtx")))

+ ;; Match exactly eight.
+ (define_predicate "const8_operand"
+   (and (match_code "const_int")
+        (match_test "INTVAL (op) == 8")))
+
  ;; Match 2, 4, or 8.  Used for leal multiplicands.
  (define_predicate "const248_operand"
    (match_code "const_int")
Index: config/i386/i386.md
===================================================================
*** config/i386/i386.md	(revision 113016)
--- config/i386/i386.md	(working copy)
***************
*** 12501,12508 ****
  (define_expand "extv"
    [(set (match_operand:SI 0 "register_operand" "")
  	(sign_extract:SI (match_operand:SI 1 "register_operand" "")
! 			 (match_operand:SI 2 "immediate_operand" "")
! 			 (match_operand:SI 3 "immediate_operand" "")))]
    ""
  {
    /* Handle extractions from %ah et al.  */
--- 12501,12508 ----
  (define_expand "extv"
    [(set (match_operand:SI 0 "register_operand" "")
  	(sign_extract:SI (match_operand:SI 1 "register_operand" "")
! 			 (match_operand:SI 2 "const8_operand" "")
! 			 (match_operand:SI 3 "const8_operand" "")))]
    ""
  {
    /* Handle extractions from %ah et al.  */
***************
*** 12518,12525 ****
  (define_expand "extzv"
    [(set (match_operand:SI 0 "register_operand" "")
  	(zero_extract:SI (match_operand 1 "ext_register_operand" "")
! 			 (match_operand:SI 2 "immediate_operand" "")
! 			 (match_operand:SI 3 "immediate_operand" "")))]
    ""
  {
    /* Handle extractions from %ah et al.  */
--- 12518,12525 ----
  (define_expand "extzv"
    [(set (match_operand:SI 0 "register_operand" "")
  	(zero_extract:SI (match_operand 1 "ext_register_operand" "")
! 			 (match_operand:SI 2 "const8_operand" "")
! 			 (match_operand:SI 3 "const8_operand" "")))]
    ""
  {
    /* Handle extractions from %ah et al.  */
***************
*** 12534,12545 ****

  (define_expand "insv"
    [(set (zero_extract (match_operand 0 "ext_register_operand" "")
! 		      (match_operand 1 "immediate_operand" "")
! 		      (match_operand 2 "immediate_operand" ""))
          (match_operand 3 "register_operand" ""))]
    ""
  {
!   /* Handle extractions from %ah et al.  */
    if (INTVAL (operands[1]) != 8 || INTVAL (operands[2]) != 8)
      FAIL;

--- 12534,12545 ----

  (define_expand "insv"
    [(set (zero_extract (match_operand 0 "ext_register_operand" "")
! 		      (match_operand 1 "const8_operand" "")
! 		      (match_operand 2 "const8_operand" ""))
          (match_operand 3 "register_operand" ""))]
    ""
  {
!   /* Handle insertions to %ah et al.  */
    if (INTVAL (operands[1]) != 8 || INTVAL (operands[2]) != 8)
      FAIL;


Roger
--