Checks
Commit Message
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
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
>
>
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
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.
@@ -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);
}
@@ -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) \
@@ -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;
}