[PATCH] Specify signed/unsigned/dontcare in calls to extract_bit_field_1.

[PATCH] Specify signed/unsigned/dontcare in calls to extract_bit_field_1.

[Roger Sayle]

[-- Attachment #1: Type: text/plain, Size: 2373 bytes --]


This patch is inspired by Jakub's work on PR rtl-optimization/110717.
The bitfield example described in comment #2, looks like:

struct S { __int128 a : 69; };
unsigned type bar (struct S *p) {
  return p->a;
}

which on x86_64 with -O2 currently generates:

bar:    movzbl  8(%rdi), %ecx
        movq    (%rdi), %rax
        andl    $31, %ecx
        movq    %rcx, %rdx
        salq    $59, %rdx
        sarq    $59, %rdx
        ret

The ANDL $31 is interesting... we first extract an unsigned 69-bit bitfield
by masking/clearing the top bits of the most significant word, and then
it gets sign-extended, by left shifting and arithmetic right shifting.
Obviously, this bit-wise AND is redundant, for signed bit-fields, we don't
require these bits to be cleared, if we're about to set them appropriately.

This patch eliminates this redundancy in the middle-end, during RTL
expansion, but extending the extract_bit_field APIs so that the integer
UNSIGNEDP argument takes a special value; 0 indicates the field should
be sign extended, 1 (any non-zero value) indicates the field should be
zero extended, but -1 indicates a third option, that we don't care how
or whether the field is extended.  By passing and checking this sentinel
value at the appropriate places we avoid the useless bit masking (on
all targets).

For the test case above, with this patch we now generate:

bar:    movzbl  8(%rdi), %ecx
        movq    (%rdi), %rax
        movq    %rcx, %rdx
        salq    $59, %rdx
        sarq    $59, %rdx
        ret

This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
and make -k check, both with and without --target_board=unix{-m32}
with no new failures.  Ok for mainline?


2023-08-03  Roger Sayle  <roger@nextmovesoftware.com>

gcc/ChangeLog
        * expmed.cc (extract_bit_field_1): Document that an UNSIGNEDP
        value of -1 is equivalent to don't care.
        (extract_integral_bit_field): Indicate that we don't require
        the most significant word to be zero extended, if we're about
        to sign extend it.
        (extract_fixed_bit_field_1): Document that an UNSIGNEDP value
        of -1 is equivalent to don't care.  Don't clear the most
        most significant bits with AND mask when UNSIGNEDP is -1.

gcc/testsuite/ChangeLog
        * gcc.target/i386/pr110717-2.c: New test case.


Thanks in advance,
Roger
--


[-- Attachment #2: patchbf2.txt --]
[-- Type: text/plain, Size: 2397 bytes --]

diff --git a/gcc/expmed.cc b/gcc/expmed.cc
index fbd4ce2..b294eabb 100644
--- a/gcc/expmed.cc
+++ b/gcc/expmed.cc
@@ -1631,6 +1631,7 @@ extract_bit_field_as_subreg (machine_mode mode, rtx op0,
 }
 
 /* A subroutine of extract_bit_field, with the same arguments.
+   If UNSIGNEDP is -1, the result need not be sign or zero extended.
    If FALLBACK_P is true, fall back to extract_fixed_bit_field
    if we can find no other means of implementing the operation.
    if FALLBACK_P is false, return NULL instead.  */
@@ -1933,7 +1934,8 @@ extract_integral_bit_field (rtx op0, opt_scalar_int_mode op0_mode,
 	  rtx result_part
 	    = extract_bit_field_1 (op0, MIN (BITS_PER_WORD,
 					     bitsize - i * BITS_PER_WORD),
-				   bitnum + bit_offset, 1, target_part,
+				   bitnum + bit_offset,
+				   (unsignedp ? 1 : -1), target_part,
 				   mode, word_mode, reverse, fallback_p, NULL);
 
 	  gcc_assert (target_part);
@@ -2187,6 +2189,7 @@ extract_fixed_bit_field (machine_mode tmode, rtx op0,
 
 /* Helper function for extract_fixed_bit_field, extracts
    the bit field always using MODE, which is the mode of OP0.
+   If UNSIGNEDP is -1, the result need not be sign or zero extended.
    The other arguments are as for extract_fixed_bit_field.  */
 
 static rtx
@@ -2231,7 +2234,8 @@ extract_fixed_bit_field_1 (machine_mode tmode, rtx op0, scalar_int_mode mode,
       /* Unless the msb of the field used to be the msb when we shifted,
 	 mask out the upper bits.  */
 
-      if (GET_MODE_BITSIZE (mode) != bitnum + bitsize)
+      if (GET_MODE_BITSIZE (mode) != bitnum + bitsize
+	  && unsignedp != -1)
 	return expand_binop (new_mode, and_optab, op0,
 			     mask_rtx (new_mode, 0, bitsize, 0),
 			     target, 1, OPTAB_LIB_WIDEN);
diff --git a/gcc/testsuite/gcc.target/i386/pr110717-2.c b/gcc/testsuite/gcc.target/i386/pr110717-2.c
new file mode 100644
index 0000000..a3cb568
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr110717-2.c
@@ -0,0 +1,20 @@
+/* PR target/110717 */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#ifdef __SIZEOF_INT128__
+#define type __int128
+#define N 59
+#else
+#define type long long
+#define N 27
+#endif
+
+struct S { type a : sizeof (type) * __CHAR_BIT__ - N; };
+
+unsigned type bar (struct S *p)
+{
+  return p->a;
+}
+
+/* { dg-final { scan-assembler-not "andl" } } */

Re: [PATCH] Specify signed/unsigned/dontcare in calls to extract_bit_field_1.

[Richard Biener]

On Thu, Aug 3, 2023 at 9:15 PM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
> This patch is inspired by Jakub's work on PR rtl-optimization/110717.
> The bitfield example described in comment #2, looks like:
>
> struct S { __int128 a : 69; };
> unsigned type bar (struct S *p) {
>   return p->a;
> }
>
> which on x86_64 with -O2 currently generates:
>
> bar:    movzbl  8(%rdi), %ecx
>         movq    (%rdi), %rax
>         andl    $31, %ecx
>         movq    %rcx, %rdx
>         salq    $59, %rdx
>         sarq    $59, %rdx
>         ret
>
> The ANDL $31 is interesting... we first extract an unsigned 69-bit bitfield
> by masking/clearing the top bits of the most significant word, and then
> it gets sign-extended, by left shifting and arithmetic right shifting.
> Obviously, this bit-wise AND is redundant, for signed bit-fields, we don't
> require these bits to be cleared, if we're about to set them appropriately.
>
> This patch eliminates this redundancy in the middle-end, during RTL
> expansion, but extending the extract_bit_field APIs so that the integer
> UNSIGNEDP argument takes a special value; 0 indicates the field should
> be sign extended, 1 (any non-zero value) indicates the field should be
> zero extended, but -1 indicates a third option, that we don't care how
> or whether the field is extended.  By passing and checking this sentinel
> value at the appropriate places we avoid the useless bit masking (on
> all targets).
>
> For the test case above, with this patch we now generate:
>
> bar:    movzbl  8(%rdi), %ecx
>         movq    (%rdi), %rax
>         movq    %rcx, %rdx
>         salq    $59, %rdx
>         sarq    $59, %rdx
>         ret
>
> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> and make -k check, both with and without --target_board=unix{-m32}
> with no new failures.  Ok for mainline?

OK.

Thanks,
Richard.

>
> 2023-08-03  Roger Sayle  <roger@nextmovesoftware.com>
>
> gcc/ChangeLog
>         * expmed.cc (extract_bit_field_1): Document that an UNSIGNEDP
>         value of -1 is equivalent to don't care.
>         (extract_integral_bit_field): Indicate that we don't require
>         the most significant word to be zero extended, if we're about
>         to sign extend it.
>         (extract_fixed_bit_field_1): Document that an UNSIGNEDP value
>         of -1 is equivalent to don't care.  Don't clear the most
>         most significant bits with AND mask when UNSIGNEDP is -1.
>
> gcc/testsuite/ChangeLog
>         * gcc.target/i386/pr110717-2.c: New test case.
>
>
> Thanks in advance,
> Roger
> --
>