On 4/15/19 7:12 AM, Christophe Lyon wrote:
On Sat, 13 Apr 2019 at 00:38, Martin Sebor <mse...@gmail.com> wrote:
On 4/12/19 3:42 PM, Jakub Jelinek wrote:
On Fri, Apr 12, 2019 at 10:45:25AM -0600, Jeff Law wrote:
gcc/ChangeLog:
PR c/89797
* targhooks.c (default_vector_alignment): Avoid assuming
argument fits in SHWI.
* tree.h (TYPE_VECTOR_SUBPARTS): Avoid sign overflow in
a shift expression.
gcc/c-family/ChangeLog:
PR c/88383
PR c/89288
PR c/89798
PR c/89797
* c-attribs.c (type_valid_for_vector_size): Detect excessively
large sizes.
...
Has the patch been tested at all?
A few times. The c-attribs.c change above didn't make it into
the commit.
Hi,
Even with r270331, I'm still seeing the ICE on aarch64 (actually with
trunk @r270370)
Is there still some commit missing?
I only fixed the TYPE_VECTOR_SUBPARTS bug behind the ICE in bug
89797 for targets where NUM_POLY_INT_COEFFS == 1, and only tested
it on x86_64.
The block of code that's compiled when NUM_POLY_INT_COEFFS == 1
is also buggy but from what I can tell in more ways than one.
inline poly_uint64
TYPE_VECTOR_SUBPARTS (const_tree node)
{
STATIC_ASSERT (NUM_POLY_INT_COEFFS <= 2);
unsigned int precision = VECTOR_TYPE_CHECK
(node)->type_common.precision;
if (NUM_POLY_INT_COEFFS == 2)
{
poly_uint64 res = 0;
res.coeffs[0] = 1 << (precision & 0xff);
if (precision & 0x100)
res.coeffs[1] = 1 << (precision & 0xff);
return res;
}
else
return (unsigned HOST_WIDE_INT)1 << precision;
}
Shifting 1 to the left by more than 30 bits is undefined (that
was also the bug I fixed in the other branch). In addition,
setting res.coeffs[1] to the same value as res.coeffs[0] doesn't
seem right either, but since precision should be less than 256
it may not actually matter. In any case, I wasn't sure and
since I don't have easy access to speedy aarch64 hardware I
wasn't brave enough to mess with it at this stage.
Then there's another bug in aarch64_simd_vector_alignment
similar to the one the patch fixed in default_vector_alignment
where the code assumes that the vector alignment fits in SHWI.
The otherwise untested patch below fixes the ICE for aarch64
but not the other (suspected) bug.
Index: gcc/tree.h
===================================================================
--- gcc/tree.h (revision 270402)
+++ gcc/tree.h (working copy)
@@ -3735,9 +3735,9 @@ TYPE_VECTOR_SUBPARTS (const_tree node)
if (NUM_POLY_INT_COEFFS == 2)
{
poly_uint64 res = 0;
- res.coeffs[0] = 1 << (precision & 0xff);
+ res.coeffs[0] = (unsigned HOST_WIDE_INT)1 << (precision & 0xff);
if (precision & 0x100)
- res.coeffs[1] = 1 << (precision & 0xff);
+ res.coeffs[1] = (unsigned HOST_WIDE_INT)1 << (precision & 0xff);
return res;
}
else
Index: gcc/config/aarch64/aarch64.c
===================================================================
--- gcc/config/aarch64/aarch64.c (revision 270402)
+++ gcc/config/aarch64/aarch64.c (working copy)
@@ -14924,7 +14924,10 @@ aarch64_simd_vector_alignment (const_tree type)
be set for non-predicate vectors of booleans. Modes are the most
direct way we have of identifying real SVE predicate types. */
return GET_MODE_CLASS (TYPE_MODE (type)) == MODE_VECTOR_BOOL ? 16
: 128;
- HOST_WIDE_INT align = tree_to_shwi (TYPE_SIZE (type));
+ tree size = TYPE_SIZE (type);
+ unsigned HOST_WIDE_INT align = 128;
+ if (tree_fits_uhwi_p (size))
+ align = tree_to_uhwi (TYPE_SIZE (type));
return MIN (align, 128);
}
I'm guessing that the full fix for TYPE_VECTOR_SUBPARTS might look
something like this, but that's just a wild guess.
===================================================================
--- gcc/tree.h (revision 270402)
+++ gcc/tree.h (working copy)
@@ -3735,9 +3735,9 @@ TYPE_VECTOR_SUBPARTS (const_tree node)
if (NUM_POLY_INT_COEFFS == 2)
{
poly_uint64 res = 0;
- res.coeffs[0] = 1 << (precision & 0xff);
+ res.coeffs[0] = (unsigned HOST_WIDE_INT)1 << (precision & 0xff);
if (precision & 0x100)
- res.coeffs[1] = 1 << (precision & 0xff);
+ res.coeffs[1] = (unsigned HOST_WIDE_INT)1 << ((precision & 0x100) >>
16);
return res;
}
else
Martin
