Message ID | ZZ5m3bq5IlVoU9fK@tucnak |
---|---|
State | New |
Headers | show |
Series | sra: Partial fix for BITINT_TYPEs [PR113120] | expand |
On Wed, 10 Jan 2024, Jakub Jelinek wrote: > Hi! > > As changed in other parts of the compiler, using > build_nonstandard_integer_type is not appropriate for arbitrary precisions, > especially if the precision comes from a BITINT_TYPE or something based on > that, build_nonstandard_integer_type relies on some integral mode being > supported that can support the precision. > > The following patch uses build_bitint_type instead for BITINT_TYPE > precisions. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? LGTM, see below for a question. > Note, it would be good if we were able to punt on the optimization > (but this code doesn't seem to be able to punt, so it needs to be done > somewhere earlier) at least in cases where building it would be invalid. > E.g. right now BITINT_TYPE can support precisions up to 65535 (inclusive), > but 65536 will not work anymore (we can't have > 16-bit TYPE_PRECISION). > I've tried to replace 513 with 65532 in the testcase and it didn't ICE, > so maybe it ran into some other SRA limit. I think SRA has a size limit, --param sra-max-scalarization-size-O{size,speed}, not sure if that is all or the one that's hit. > 2024-01-10 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/113120 > * tree-sra.cc (analyze_access_subtree): For BITINT_TYPE > with root->size TYPE_PRECISION don't build anything new. > Otherwise, if root->type is a BITINT_TYPE, use build_bitint_type > rather than build_nonstandard_integer_type. > > * gcc.dg/bitint-63.c: New test. > > --- gcc/tree-sra.cc.jj 2024-01-03 11:51:35.054682295 +0100 > +++ gcc/tree-sra.cc 2024-01-09 19:50:42.911500487 +0100 > @@ -2733,7 +2733,8 @@ analyze_access_subtree (struct access *r > For integral types this means the precision has to match. > Avoid assumptions based on the integral type kind, too. */ > if (INTEGRAL_TYPE_P (root->type) > - && (TREE_CODE (root->type) != INTEGER_TYPE > + && ((TREE_CODE (root->type) != INTEGER_TYPE > + && TREE_CODE (root->type) != BITINT_TYPE) > || TYPE_PRECISION (root->type) != root->size) > /* But leave bitfield accesses alone. */ > && (TREE_CODE (root->expr) != COMPONENT_REF > @@ -2742,8 +2743,11 @@ analyze_access_subtree (struct access *r > tree rt = root->type; > gcc_assert ((root->offset % BITS_PER_UNIT) == 0 > && (root->size % BITS_PER_UNIT) == 0); > - root->type = build_nonstandard_integer_type (root->size, > - TYPE_UNSIGNED (rt)); > + if (TREE_CODE (root->type) == BITINT_TYPE) > + root->type = build_bitint_type (root->size, TYPE_UNSIGNED (rt)); I suppose we don't exactly need to preserve BITINT-ness, say if root->size fits the largest supported integer mode? It's OK as-is for now. > + else > + root->type = build_nonstandard_integer_type (root->size, > + TYPE_UNSIGNED (rt)); > root->expr = build_ref_for_offset (UNKNOWN_LOCATION, root->base, > root->offset, root->reverse, > root->type, NULL, false); > --- gcc/testsuite/gcc.dg/bitint-63.c.jj 2024-01-09 20:08:04.831720434 +0100 > +++ gcc/testsuite/gcc.dg/bitint-63.c 2024-01-09 20:07:43.045029421 +0100 > @@ -0,0 +1,24 @@ > +/* PR tree-optimization/113120 */ > +/* { dg-do compile { target bitint } } */ > +/* { dg-require-stack-check "generic" } */ > +/* { dg-options "-std=c23 -O -fno-tree-fre --param=large-stack-frame=1024 -fstack-check=generic" } */ > + > +#if __BITINT_MAXWIDTH__ >= 513 > +typedef _BitInt(513) B; > +#else > +typedef int B; > +#endif > + > +static inline __attribute__((__always_inline__)) void > +bar (B x) > +{ > + B y = x; > + if (y) > + __builtin_abort (); > +} > + > +void > +foo (void) > +{ > + bar (0); > +} > > Jakub > >
On Wed, Jan 10, 2024 at 10:51:32AM +0100, Richard Biener wrote: > > @@ -2742,8 +2743,11 @@ analyze_access_subtree (struct access *r > > tree rt = root->type; > > gcc_assert ((root->offset % BITS_PER_UNIT) == 0 > > && (root->size % BITS_PER_UNIT) == 0); > > - root->type = build_nonstandard_integer_type (root->size, > > - TYPE_UNSIGNED (rt)); > > + if (TREE_CODE (root->type) == BITINT_TYPE) > > + root->type = build_bitint_type (root->size, TYPE_UNSIGNED (rt)); > > I suppose we don't exactly need to preserve BITINT-ness, say if > root->size fits the largest supported integer mode? It's OK as-is Sure, we could use INTEGER_TYPE in that case, but if we use BITINT_TYPE, it won't do harm either, worst case it will be lowered to those INTEGER_TYPEs later again. What is IMHO important is not to introduce BITINT_TYPEs where they weren't used before, we didn't need to use them before either. And to use BITINT_TYPEs for large ones which can't be expressed in INTEGER_TYPEs. Jakub
Hi, On Wed, Jan 10 2024, Jakub Jelinek wrote: > Hi! > > As changed in other parts of the compiler, using > build_nonstandard_integer_type is not appropriate for arbitrary precisions, > especially if the precision comes from a BITINT_TYPE or something based on > that, build_nonstandard_integer_type relies on some integral mode being > supported that can support the precision. > > The following patch uses build_bitint_type instead for BITINT_TYPE > precisions. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > Note, it would be good if we were able to punt on the optimization > (but this code doesn't seem to be able to punt, so it needs to be done > somewhere earlier) at least in cases where building it would be invalid. > E.g. right now BITINT_TYPE can support precisions up to 65535 (inclusive), > but 65536 will not work anymore (we can't have > 16-bit TYPE_PRECISION). > I've tried to replace 513 with 65532 in the testcase and it didn't ICE, > so maybe it ran into some other SRA limit. Thank you very much for the patch. Regarding punting, did you mean for all BITINT_TYPEs or just for big ones, like you did when you fixed PR 11333 (thanks for that too) or something entirely else? Martin > > 2024-01-10 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/113120 > * tree-sra.cc (analyze_access_subtree): For BITINT_TYPE > with root->size TYPE_PRECISION don't build anything new. > Otherwise, if root->type is a BITINT_TYPE, use build_bitint_type > rather than build_nonstandard_integer_type. > > * gcc.dg/bitint-63.c: New test.
On Wed, Jan 17, 2024 at 03:46:44PM +0100, Martin Jambor wrote: > > Note, it would be good if we were able to punt on the optimization > > (but this code doesn't seem to be able to punt, so it needs to be done > > somewhere earlier) at least in cases where building it would be invalid. > > E.g. right now BITINT_TYPE can support precisions up to 65535 (inclusive), > > but 65536 will not work anymore (we can't have > 16-bit TYPE_PRECISION). > > I've tried to replace 513 with 65532 in the testcase and it didn't ICE, > > so maybe it ran into some other SRA limit. > > Thank you very much for the patch. Regarding punting, did you mean for > all BITINT_TYPEs or just for big ones, like you did when you fixed PR > 11333 (thanks for that too) or something entirely else? I meant what I did in PR113330, but still wonder if we really need to use a root->size which is multiple of BITS_PER_UNIT (or words or whatever it actually is), at least on little endian if the _BitInt starts at the start of a memory. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113408#c1 for more details, wonder if it just couldn't use _BitInt(713) in there directly rather than _BitInt(768). Jakub
--- gcc/tree-sra.cc.jj 2024-01-03 11:51:35.054682295 +0100 +++ gcc/tree-sra.cc 2024-01-09 19:50:42.911500487 +0100 @@ -2733,7 +2733,8 @@ analyze_access_subtree (struct access *r For integral types this means the precision has to match. Avoid assumptions based on the integral type kind, too. */ if (INTEGRAL_TYPE_P (root->type) - && (TREE_CODE (root->type) != INTEGER_TYPE + && ((TREE_CODE (root->type) != INTEGER_TYPE + && TREE_CODE (root->type) != BITINT_TYPE) || TYPE_PRECISION (root->type) != root->size) /* But leave bitfield accesses alone. */ && (TREE_CODE (root->expr) != COMPONENT_REF @@ -2742,8 +2743,11 @@ analyze_access_subtree (struct access *r tree rt = root->type; gcc_assert ((root->offset % BITS_PER_UNIT) == 0 && (root->size % BITS_PER_UNIT) == 0); - root->type = build_nonstandard_integer_type (root->size, - TYPE_UNSIGNED (rt)); + if (TREE_CODE (root->type) == BITINT_TYPE) + root->type = build_bitint_type (root->size, TYPE_UNSIGNED (rt)); + else + root->type = build_nonstandard_integer_type (root->size, + TYPE_UNSIGNED (rt)); root->expr = build_ref_for_offset (UNKNOWN_LOCATION, root->base, root->offset, root->reverse, root->type, NULL, false); --- gcc/testsuite/gcc.dg/bitint-63.c.jj 2024-01-09 20:08:04.831720434 +0100 +++ gcc/testsuite/gcc.dg/bitint-63.c 2024-01-09 20:07:43.045029421 +0100 @@ -0,0 +1,24 @@ +/* PR tree-optimization/113120 */ +/* { dg-do compile { target bitint } } */ +/* { dg-require-stack-check "generic" } */ +/* { dg-options "-std=c23 -O -fno-tree-fre --param=large-stack-frame=1024 -fstack-check=generic" } */ + +#if __BITINT_MAXWIDTH__ >= 513 +typedef _BitInt(513) B; +#else +typedef int B; +#endif + +static inline __attribute__((__always_inline__)) void +bar (B x) +{ + B y = x; + if (y) + __builtin_abort (); +} + +void +foo (void) +{ + bar (0); +}