[RFC,WIP] _BitInt bit-field support [PR102989]

Message ID ZMPpap9VO0EMUGCn@tucnak
State Unresolved
Headers
Series [RFC,WIP] _BitInt bit-field support [PR102989] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek July 28, 2023, 4:14 p.m. UTC
  On Fri, Jul 28, 2023 at 11:05:42AM +0200, Jakub Jelinek via Gcc-patches wrote:
> On Thu, Jul 27, 2023 at 06:41:44PM +0000, Joseph Myers wrote:
> > On Thu, 27 Jul 2023, Jakub Jelinek via Gcc-patches wrote:
> > 
> > > - _BitInt(N) bit-fields aren't supported yet (the patch rejects them); I'd like
> > >   to enable those incrementally, but don't really see details on how such
> > >   bit-fields should be laid-out in memory nor passed inside of function
> > >   arguments; LLVM implements something, but it is a question if that is what
> > >   the various ABIs want
> > 
> > So if the x86-64 ABI (or any other _BitInt ABI that already exists) 
> > doesn't specify this adequately then an issue should be filed (at 
> > https://gitlab.com/x86-psABIs/x86-64-ABI/-/issues in the x86-64 case).
> > 
> > (Note that the language specifies that e.g. _BitInt(123):45 gets promoted 
> > to _BitInt(123) by the integer promotions, rather than left as a type with 
> > the bit-field width.)
> 
> Ok, I'll try to investigate in detail what LLVM does and what GCC would do
> if I just enabled the bitfield support and report.  Still, I'd like to
> handle this only in incremental step after the rest of _BitInt support goes
> in.

So, I've spent some time on this but after simply enabling _BitInt
bit-fields everything I've tried yields the same layout with both GCC and
LLVM trunk and as I didn't have to muck with stor-layout.cc for that
(haven't tried yet the function arg passing/returning), I assume it is just
the generic PCC_BITFIELD_TYPE_MATTERS behavior which works like that, so
while it wouldn't hurt it the psABI said something about those, perhaps it is
ok as is.

But I ran into a compiler divergence on _Generic with bit-field expressions.
My understanding is that _Generic controlling expression undergoes array
to pointer and function to pointer conversions, but not integral promotions
(otherwise it would never match for char, short etc. types).
C23 draft I have says:
"A bit-field is interpreted as having a signed or unsigned integer type
consisting of the specified number of bits"
but doesn't say which exact signed or unsigned integer type that is.
Additionally, I think at least for the larger widths, it would be really
strange if it was interpreted as an INTEGER_TYPE with say precision of 350
because that is more than INTEGER_TYPEs can really use.
So, in the patch I try to use a bit-precise integer type for the bit-field
type if it has a bit-precise bit-field type if possible (where not possible
is the special case of signed 1-bit field where _BitInt(1) is not allowed.

Now, in the testcase with GCC the
static_assert (expr_has_type (s4.a, _BitInt(195)));
static_assert (expr_has_type (s4.b, _BitInt(282)));
static_assert (expr_has_type (s4.c, _BitInt(389)));
static_assert (expr_has_type (s4.d, _BitInt(2)));
static_assert (expr_has_type (s5.a, _BitInt(192)));
static_assert (expr_has_type (s5.b, unsigned _BitInt(192)));
static_assert (expr_has_type (s5.c, _BitInt(192)));
static_assert (expr_has_type (s6.a, _BitInt(2)));
assertions all fail (and all the ones where integer promotions are performed
for binary operation succeed).  They would succeed with
static_assert (expr_has_type (s4.a, _BitInt(63)));
static_assert (expr_has_type (s4.b, _BitInt(280)));
static_assert (expr_has_type (s4.c, _BitInt(23)));
static_assert (!expr_has_type (s4.d, _BitInt(2)));
static_assert (expr_has_type (s5.a, _BitInt(191)));
static_assert (expr_has_type (s5.b, unsigned _BitInt(190)));
static_assert (expr_has_type (s5.c, _BitInt(189)));
static_assert (!expr_has_type (s6.a, _BitInt(2)));
The s4.d and s6.a cases for GCC with this patch actually have int:1 type,
something that can't be ever matched in _Generic except for default:.

On the other side, all the above pass with LLVM, i.e. as if they have
undergone the integral promotion for the _BitInt bitfield case even for
_Generic.  And the
static_assert (expr_has_type (s4.c + 1uwb, _BitInt(389)));
static_assert (expr_has_type (s4.d * 0wb, _BitInt(2)));
static_assert (expr_has_type (s6.a + 0wb, _BitInt(2)));
That looks to me like LLVM bug, because
"The value from a bit-field of a bit-precise integer type is converted to
the corresponding bit-precise integer type."
specifies that s4.c has _BitInt(389) type after integer promotions
and s4.d and s6.a have _BitInt(2) type.  Now, 1uwb has unsigned _BitInt(1)
type and 0wb has _BitInt(2) and the common type for those in all cases is
I believe the type of the left operand.

Thoughts on this?

The patch is obviously incomplete, I haven't added code for lowering
loads/stores from/to bit-fields for large/huge _BitInt nor added testcase
coverage for passing of small structs with _BitInt bit-fields as function
arguments/return values.

2023-07-28  Jakub Jelinek  <jakub@redhat.com>

	PR c/102989
	* c-typeck.cc (perform_integral_promotions): Promote bit-fields
	with bit-precise integral types to those types.
	* c-decl.cc (check_bitfield_type_and_width): Allow _BitInt bit-fields.
	(finish_struct): If field's bit-field type is BITINT_TYPE, use
	BITINT_TYPE as its type if possible.

	* gcc.dg/bitint-17.c: New test.



	Jakub
  

Comments

Joseph Myers July 28, 2023, 6:37 p.m. UTC | #1
On Fri, 28 Jul 2023, Jakub Jelinek via Gcc-patches wrote:

> But I ran into a compiler divergence on _Generic with bit-field expressions.
> My understanding is that _Generic controlling expression undergoes array
> to pointer and function to pointer conversions, but not integral promotions
> (otherwise it would never match for char, short etc. types).
> C23 draft I have says:
> "A bit-field is interpreted as having a signed or unsigned integer type
> consisting of the specified number of bits"
> but doesn't say which exact signed or unsigned integer type that is.

Yes, the type used in _Generic isn't fully specified, just the type after 
integer promotions in contexts where those occur.

> static_assert (expr_has_type (s4.c + 1uwb, _BitInt(389)));
> static_assert (expr_has_type (s4.d * 0wb, _BitInt(2)));
> static_assert (expr_has_type (s6.a + 0wb, _BitInt(2)));
> That looks to me like LLVM bug, because
> "The value from a bit-field of a bit-precise integer type is converted to
> the corresponding bit-precise integer type."
> specifies that s4.c has _BitInt(389) type after integer promotions
> and s4.d and s6.a have _BitInt(2) type.  Now, 1uwb has unsigned _BitInt(1)
> type and 0wb has _BitInt(2) and the common type for those in all cases is
> I believe the type of the left operand.

Indeed, I'd expect those to pass, since in those cases integer promotions 
(to the declared _BitInt type of the bit-field, without the bit-field 
width) are applied.
  

Patch

--- gcc/c/c-typeck.cc.jj	2023-07-11 15:28:54.708679456 +0200
+++ gcc/c/c-typeck.cc	2023-07-28 17:12:19.720007447 +0200
@@ -2279,12 +2279,17 @@  perform_integral_promotions (tree exp)
   /* ??? This should no longer be needed now bit-fields have their
      proper types.  */
   if (TREE_CODE (exp) == COMPONENT_REF
-      && DECL_C_BIT_FIELD (TREE_OPERAND (exp, 1))
+      && DECL_C_BIT_FIELD (TREE_OPERAND (exp, 1)))
+    {
+      if (TREE_CODE (DECL_BIT_FIELD_TYPE (TREE_OPERAND (exp, 1)))
+	  == BITINT_TYPE)
+	return convert (DECL_BIT_FIELD_TYPE (TREE_OPERAND (exp, 1)), exp);
       /* If it's thinner than an int, promote it like a
 	 c_promoting_integer_type_p, otherwise leave it alone.  */
-      && compare_tree_int (DECL_SIZE (TREE_OPERAND (exp, 1)),
-			   TYPE_PRECISION (integer_type_node)) < 0)
-    return convert (integer_type_node, exp);
+      if (compare_tree_int (DECL_SIZE (TREE_OPERAND (exp, 1)),
+			    TYPE_PRECISION (integer_type_node)) < 0)
+	return convert (integer_type_node, exp);
+    }
 
   if (c_promoting_integer_type_p (type))
     {
--- gcc/c/c-decl.cc.jj	2023-07-11 15:28:54.706679483 +0200
+++ gcc/c/c-decl.cc	2023-07-28 17:15:46.480173360 +0200
@@ -6382,7 +6382,8 @@  check_bitfield_type_and_width (location_
   /* Detect invalid bit-field type.  */
   if (TREE_CODE (*type) != INTEGER_TYPE
       && TREE_CODE (*type) != BOOLEAN_TYPE
-      && TREE_CODE (*type) != ENUMERAL_TYPE)
+      && TREE_CODE (*type) != ENUMERAL_TYPE
+      && TREE_CODE (*type) != BITINT_TYPE)
     {
       error_at (loc, "bit-field %qs has invalid type", name);
       *type = unsigned_type_node;
@@ -9322,8 +9323,14 @@  finish_struct (location_t loc, tree t, t
 	  tree type = TREE_TYPE (field);
 	  if (width != TYPE_PRECISION (type))
 	    {
-	      TREE_TYPE (field)
-		= c_build_bitfield_integer_type (width, TYPE_UNSIGNED (type));
+	      if (TREE_CODE (type) == BITINT_TYPE
+		  && (width > 1 || TYPE_UNSIGNED (type)))
+		TREE_TYPE (field)
+		  = build_bitint_type (width, TYPE_UNSIGNED (type));
+	      else
+		TREE_TYPE (field)
+		  = c_build_bitfield_integer_type (width,
+						   TYPE_UNSIGNED (type));
 	      SET_DECL_MODE (field, TYPE_MODE (TREE_TYPE (field)));
 	    }
 	  DECL_INITIAL (field) = NULL_TREE;
--- gcc/testsuite/gcc.dg/bitint-17.c.jj	2023-07-28 15:09:19.199372151 +0200
+++ gcc/testsuite/gcc.dg/bitint-17.c	2023-07-28 17:29:25.330961298 +0200
@@ -0,0 +1,55 @@ 
+/* PR c/102989 */
+/* { dg-do compile { target bitint } } */
+/* { dg-options "-O2 -std=c2x -pedantic-errors" } */
+
+#define expr_has_type(e, t) _Generic (e, default : 0, t : 1)
+
+struct S1 { char x; char : 0; char y; };
+struct S2 { char x; int : 0; char y; };
+#if __BITINT_MAXWIDTH__ >= 575
+struct S3 { char x; _BitInt(575) : 0; char y; };
+#endif
+#if __BITINT_MAXWIDTH__ >= 389
+struct S4 { char x; _BitInt(195) a : 63; _BitInt(282) b : 280; _BitInt(389) c : 23; _BitInt(2) d : 1; char y; };
+#endif
+#if __BITINT_MAXWIDTH__ >= 192
+struct S5 { char x; _BitInt(192) a : 191; unsigned _BitInt(192) b : 190; _BitInt(192) c : 189; char y; };
+#endif
+struct S6 { _BitInt(2) a : 1; };
+#if __BITINT_MAXWIDTH__ >= 389
+struct S4 s4;
+static_assert (expr_has_type (s4.a, _BitInt(195)));
+static_assert (expr_has_type (s4.a + 1uwb, _BitInt(195)));
+static_assert (expr_has_type (s4.b, _BitInt(282)));
+static_assert (expr_has_type (s4.b + 1uwb, _BitInt(282)));
+static_assert (expr_has_type (s4.c, _BitInt(389)));
+static_assert (expr_has_type (s4.c + 1uwb, _BitInt(389)));
+static_assert (expr_has_type (s4.d, _BitInt(2)));
+static_assert (expr_has_type (s4.d * 0wb, _BitInt(2)));
+#endif
+#if __BITINT_MAXWIDTH__ >= 192
+struct S5 s5;
+static_assert (expr_has_type (s5.a, _BitInt(192)));
+static_assert (expr_has_type (s5.a + 1uwb, _BitInt(192)));
+static_assert (expr_has_type (s5.b, unsigned _BitInt(192)));
+static_assert (expr_has_type (s5.b + 1wb, unsigned _BitInt(192)));
+static_assert (expr_has_type (s5.c, _BitInt(192)));
+static_assert (expr_has_type (s5.c + 1uwb, _BitInt(192)));
+#endif
+struct S6 s6;
+static_assert (expr_has_type (s6.a, _BitInt(2)));
+static_assert (expr_has_type (s6.a + 0wb, _BitInt(2)));
+#if defined(__x86_64__) && __LP64__ && __BITINT_MAXWIDTH__ >= 575
+static_assert (sizeof (struct S1) == 2);
+static_assert (sizeof (struct S2) == 5);
+static_assert (sizeof (struct S3) == 9);
+static_assert (sizeof (struct S4) == 48);
+static_assert (sizeof (struct S5) == 88);
+static_assert (sizeof (struct S6) == 1);
+static_assert (alignof (struct S1) == 1);
+static_assert (alignof (struct S2) == 1);
+static_assert (alignof (struct S3) == 1);
+static_assert (alignof (struct S4) == 8);
+static_assert (alignof (struct S5) == 8);
+static_assert (alignof (struct S6) == 1);
+#endif