diff mbox series

small _BitInt tweaks

Message ID ZP9RahtJvkQl7PFG@tucnak
State Unresolved
Headers
Series small _BitInt tweaks |

Checks

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

Commit Message

Jakub Jelinek Sept. 11, 2023, 5:42 p.m. UTC 
  Hi!

When discussing PR111369 with Andrew Pinski, I've realized that
I haven't added BITINT_TYPE handling to range_check_type.  Right now
(unsigned) max + 1 == (unsigned) min for signed _BitInt,l so I think we
don't need to do the extra hops for BITINT_TYPE (though possibly we don't
need them for INTEGER_TYPE either in the two's complement word and we don't
support anything else, though I really don't know if Ada or some other
FEs don't create weird INTEGER_TYPEs).
And, also I think it is undesirable when being asked for signed_type_for
of unsigned _BitInt(1) (which is valid) to get signed _BitInt(1) (which is
invalid, the standard only allows signed _BitInt(2) and larger), so the
patch returns 1-bit signed INTEGER_TYPE for those cases.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2023-09-11  Jakub Jelinek  <jakub@redhat.com>

gcc/
	* tree.cc (signed_or_unsigned_type_for): Return INTEGER_TYPE for
	signed variant of unsigned _BitInt(1).
	* fold-const.cc (range_check_type): Handle BITINT_TYPE like
	OFFSET_TYPE.
gcc/c-family/
	* c-common.cc (c_common_signed_or_unsigned_type): Return INTEGER_TYPE
	for signed variant of unsigned _BitInt(1).


	Jakub

Comments

Richard Biener Sept. 12, 2023, 10:27 a.m. UTC | #1 
On Mon, 11 Sep 2023, Jakub Jelinek wrote:

> Hi!
> 
> When discussing PR111369 with Andrew Pinski, I've realized that
> I haven't added BITINT_TYPE handling to range_check_type.  Right now
> (unsigned) max + 1 == (unsigned) min for signed _BitInt,l so I think we
> don't need to do the extra hops for BITINT_TYPE (though possibly we don't
> need them for INTEGER_TYPE either in the two's complement word and we don't
> support anything else, though I really don't know if Ada or some other
> FEs don't create weird INTEGER_TYPEs).
> And, also I think it is undesirable when being asked for signed_type_for
> of unsigned _BitInt(1) (which is valid) to get signed _BitInt(1) (which is
> invalid, the standard only allows signed _BitInt(2) and larger), so the
> patch returns 1-bit signed INTEGER_TYPE for those cases.

I think the last bit is a bit surprising - do the frontends use
signed_or_unsigned_type_for and would they be confused if getting
back an INTEGER_TYPE here?

The range_check_type bits are OK.  For the tree.cc part I think
the middle-end can just handle signed 1-bit BITINT fine?

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2023-09-11  Jakub Jelinek  <jakub@redhat.com>
> 
> gcc/
> 	* tree.cc (signed_or_unsigned_type_for): Return INTEGER_TYPE for
> 	signed variant of unsigned _BitInt(1).
> 	* fold-const.cc (range_check_type): Handle BITINT_TYPE like
> 	OFFSET_TYPE.
> gcc/c-family/
> 	* c-common.cc (c_common_signed_or_unsigned_type): Return INTEGER_TYPE
> 	for signed variant of unsigned _BitInt(1).
> 
> --- gcc/tree.cc.jj	2023-09-06 17:50:30.707589026 +0200
> +++ gcc/tree.cc	2023-09-11 16:24:58.749625569 +0200
> @@ -11096,7 +11096,7 @@ signed_or_unsigned_type_for (int unsigne
>    else
>      return NULL_TREE;
>  
> -  if (TREE_CODE (type) == BITINT_TYPE)
> +  if (TREE_CODE (type) == BITINT_TYPE && (unsignedp || bits > 1))
>      return build_bitint_type (bits, unsignedp);
>    return build_nonstandard_integer_type (bits, unsignedp);
>  }
> --- gcc/c-family/c-common.cc.jj	2023-09-06 17:34:24.467254960 +0200
> +++ gcc/c-family/c-common.cc	2023-09-11 16:24:07.873300311 +0200
> @@ -2739,7 +2739,9 @@ c_common_signed_or_unsigned_type (int un
>        || TYPE_UNSIGNED (type) == unsignedp)
>      return type;
>  
> -  if (TREE_CODE (type) == BITINT_TYPE)
> +  if (TREE_CODE (type) == BITINT_TYPE
> +      /* signed _BitInt(1) is invalid, avoid creating that.  */
> +      && (unsignedp || TYPE_PRECISION (type) > 1))
>      return build_bitint_type (TYPE_PRECISION (type), unsignedp);
>  
>  #define TYPE_OK(node)							    \
> --- gcc/fold-const.cc.jj	2023-09-11 11:05:47.473728473 +0200
> +++ gcc/fold-const.cc	2023-09-11 16:28:06.052141516 +0200
> @@ -5565,7 +5565,12 @@ range_check_type (tree etype)
>        else
>  	return NULL_TREE;
>      }
> -  else if (POINTER_TYPE_P (etype) || TREE_CODE (etype) == OFFSET_TYPE)
> +  else if (POINTER_TYPE_P (etype)
> +	   || TREE_CODE (etype) == OFFSET_TYPE
> +	   /* Right now all BITINT_TYPEs satisfy
> +	      (unsigned) max + 1 == (unsigned) min, so no need to verify
> +	      that like for INTEGER_TYPEs.  */
> +	   || TREE_CODE (etype) == BITINT_TYPE)
>      etype = unsigned_type_for (etype);
>    return etype;
>  }
> 
> 	Jakub
> 
>
Jakub Jelinek Sept. 12, 2023, 11:26 a.m. UTC | #2 
On Tue, Sep 12, 2023 at 10:27:18AM +0000, Richard Biener wrote:
> On Mon, 11 Sep 2023, Jakub Jelinek wrote:
> > And, also I think it is undesirable when being asked for signed_type_for
> > of unsigned _BitInt(1) (which is valid) to get signed _BitInt(1) (which is
> > invalid, the standard only allows signed _BitInt(2) and larger), so the
> > patch returns 1-bit signed INTEGER_TYPE for those cases.
> 
> I think the last bit is a bit surprising - do the frontends use
> signed_or_unsigned_type_for and would they be confused if getting
> back an INTEGER_TYPE here?

I see a single c-family/c-pretty-print.cc use of signed_or_unsigned_type_for
and none of signed_type_for in the C/C++ FEs (unsigned_type_for is used in a
couple of spots, but that isn't affected), c_common_signed_type
or c_common_signed_or_unsigned_type is used more than that, but I still
think it is mostly used for warning stuff and similar or when called with
some specific types like sizetype.  I don't think the FE uses (or should
use) those functions to decide e.g. on types of expressions etc., that is
what common_type etc. are for.
And, for the very small precisions the distinction between BITINT_TYPE and
INTEGER_TYPE should be limited to just loads/stores from memory (in case
there are different rules for what to do with padding bits in those cases if
any) and on function arguments/return values, I think none of this is really
affected by those signed_type_for/c_common_signed_type results.

And by ensuring we never create 1-bit signed BITINT_TYPE e.g. the backends
don't need to worry about them.

But I admit I don't feel strongly about that.

Joseph, what do you think about this?

	Jakub
Joseph Myers Sept. 12, 2023, 5:27 p.m. UTC | #3 
On Tue, 12 Sep 2023, Jakub Jelinek via Gcc-patches wrote:

> And by ensuring we never create 1-bit signed BITINT_TYPE e.g. the backends
> don't need to worry about them.
> 
> But I admit I don't feel strongly about that.
> 
> Joseph, what do you think about this?

I think it's appropriate to avoid 1-bit signed BITINT_TYPE consistently.
diff mbox series

Patch

--- gcc/tree.cc.jj	2023-09-06 17:50:30.707589026 +0200
+++ gcc/tree.cc	2023-09-11 16:24:58.749625569 +0200
@@ -11096,7 +11096,7 @@  signed_or_unsigned_type_for (int unsigne
   else
     return NULL_TREE;
 
-  if (TREE_CODE (type) == BITINT_TYPE)
+  if (TREE_CODE (type) == BITINT_TYPE && (unsignedp || bits > 1))
     return build_bitint_type (bits, unsignedp);
   return build_nonstandard_integer_type (bits, unsignedp);
 }
--- gcc/c-family/c-common.cc.jj	2023-09-06 17:34:24.467254960 +0200
+++ gcc/c-family/c-common.cc	2023-09-11 16:24:07.873300311 +0200
@@ -2739,7 +2739,9 @@  c_common_signed_or_unsigned_type (int un
       || TYPE_UNSIGNED (type) == unsignedp)
     return type;
 
-  if (TREE_CODE (type) == BITINT_TYPE)
+  if (TREE_CODE (type) == BITINT_TYPE
+      /* signed _BitInt(1) is invalid, avoid creating that.  */
+      && (unsignedp || TYPE_PRECISION (type) > 1))
     return build_bitint_type (TYPE_PRECISION (type), unsignedp);
 
 #define TYPE_OK(node)							    \
--- gcc/fold-const.cc.jj	2023-09-11 11:05:47.473728473 +0200
+++ gcc/fold-const.cc	2023-09-11 16:28:06.052141516 +0200
@@ -5565,7 +5565,12 @@  range_check_type (tree etype)
       else
 	return NULL_TREE;
     }
-  else if (POINTER_TYPE_P (etype) || TREE_CODE (etype) == OFFSET_TYPE)
+  else if (POINTER_TYPE_P (etype)
+	   || TREE_CODE (etype) == OFFSET_TYPE
+	   /* Right now all BITINT_TYPEs satisfy
+	      (unsigned) max + 1 == (unsigned) min, so no need to verify
+	      that like for INTEGER_TYPEs.  */
+	   || TREE_CODE (etype) == BITINT_TYPE)
     etype = unsigned_type_for (etype);
   return etype;
 }