Bug 96377 - [10/11 Regression] GCC 10.2/11 doesn't build Linux kernel anymore
Summary: [10/11 Regression] GCC 10.2/11 doesn't build Linux kernel anymore
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 10.0
: P1 normal
Target Milestone: 10.3
Assignee: Richard Sandiford
URL:
Keywords: rejects-valid
: 96990 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-07-29 16:04 UTC by Jakub Jelinek
Modified: 2021-01-27 12:07 UTC (History)
8 users (show)

See Also:
Host:
Target:
Build:
Known to work: 10.1.0
Known to fail: 10.2.0, 11.0
Last reconfirmed: 2020-07-29 00:00:00


Attachments
Patch for the initialisation problem (3.33 KB, patch)
2020-07-30 17:08 UTC, Richard Sandiford
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2020-07-29 16:04:25 UTC
The following testcase reduced from Linux kernel's crypto/aegis128-neon-inner.c
started to be rejected with r11-1741-g31427b974ed7b7dd54e28fec595e731bf6eea8ba and r10-8501-g932e9140d3268cf2033c1c3e93219541c53fcd29

#include <arm_neon.h>

struct aegis128_state {
 uint8x16_t v[5];
};

void foo(const void *key, const void *iv, const void *const0, const void *const1)
{
 uint8x16_t k = vld1q_u8(key);
 uint8x16_t kiv = k ^ vld1q_u8(iv);
 struct aegis128_state st = {{
  kiv,
  vld1q_u8(const1),
  vld1q_u8(const0),
  k ^ vld1q_u8(const0),
  k ^ vld1q_u8(const1),
 }};
}

The error is:
error: incompatible types when initializing type ‘unsigned char’ using type ‘uint8x16_t’ {aka ‘__Uint8x16_t’}
(twice)
Comment 1 Jakub Jelinek 2020-07-29 16:18:07 UTC
I think the problem is that c_common_type does:
742	  if (TYPE_ATTRIBUTES (t1) != NULL_TREE)
743	    t1 = build_type_attribute_variant (t1, NULL_TREE);
744	
745	  if (TYPE_ATTRIBUTES (t2) != NULL_TREE)
746	    t2 = build_type_attribute_variant (t2, NULL_TREE);
which strips off the "Advanced SIMD type" attribute from both of the types.
C++ FE doesn't do anything like that.
Comment 2 Jakub Jelinek 2020-07-29 16:40:55 UTC
This dates back to https://gcc.gnu.org/legacy-ml/gcc-patches/2004-06/msg00288.html
Dunno, do we want to never strip attributes from VECTOR_TYPEs and only strip them that way from non-VECTOR_TYPEs?  Or only strip some attributes and keep others?
Or just add a if (t1 == t2) return t1; before the attribute stripping (perhaps for VECTOR_TYPEs only)?
Note, due to the typedef __Uint8x16_t uint8x16_t; on this testcase t1 and t2 are equal and are the typedef file, and because they are unqualified, we don't use TYPE_MAIN_VARIANT (which is the __Uint8x16_t type).
What is the desirable behavior for the common type if one is the aarch64 specific vector and the other generic vector with the same mode?
The C++ behavior for vector types in cp_common_type is that it will merge attributes from both types, and if one is TYPE_UNSIGNED, pick that one, otherwise pick the other one.  So in that case the ARM specific vector type wins.

I must say I'm surprised that the 10.2 version of the patch also matters, perhaps the FE isn't using the target hook to compare attributes, but rather just compares TYPE_MAIN_VARIANT or something similar to determine if the initializer is initializing a whole vector or an element of the vector.
Comment 3 Jakub Jelinek 2020-07-29 16:48:18 UTC
For those that need a quick workaround for the kernel, I think
  (uint8x16_t) (k ^ vld1q_u8(const0)),
  (uint8x16_t) (k ^ vld1q_u8(const1)),
instead of
  k ^ vld1q_u8(const0),
  k ^ vld1q_u8(const1),
will do the job.  And it is a question if portable code can use binary operators on the ARM specific vector types rather than intrinsics or the normal generic vectors instead.
Comment 4 Richard Sandiford 2020-07-29 17:40:41 UTC
Gah.  Thanks for the report and analysis.  Like you say,
I was hoping that adding the attributes in 10.2 but not
making them matter for type compatibility would have
been pretty conservative, but obviously it wasn't
conservative enough.

I'll take a look tomorrow.
Comment 5 Richard Sandiford 2020-07-30 14:06:19 UTC
I think this is bound up with the question whether:

typedef int v4si __attribute__((vector_size(16)));
typedef short v8hi __attribute__((vector_size(16)));

struct s {
  v8hi x;
  v4si y;
};

void
foo (v4si i, v8hi h)
{
  struct s x1 = { i, i };
  struct s x2 = { h, h };
  struct s x3 = { i, h };
  struct s x4 = { h, i };
}

should be valid with -flax-vector-conversions.  g++ and clang
think it should be, but gcc doesn't accept it.

IMO process_init_element shouldn't recurse into vector types
if the initialisation value is also a vector type.  We should
treat the vector value as initialising the whole type and report
an error if they're not compatible.
Comment 6 Richard Sandiford 2020-07-30 14:09:02 UTC
Joseph, Marek, any thoughts on comment #5?
Comment 7 Jakub Jelinek 2020-07-30 15:41:39 UTC
I guess that is reasonable thing to do, if the two vector types aren't really compatible one will get an error.
But then, for trunk, won't the stripping of the attributes from vector types still mean that comp_type_attributes will return false when comparing the destination vector type (the uint8x16_t one) with the one from the binary expression (the same with stripped attribute, i.e. essentially a generic vector) and thus the initialization will be considered erroneous?
Comment 8 Richard Sandiford 2020-07-30 15:58:09 UTC
(In reply to Jakub Jelinek from comment #7)
> I guess that is reasonable thing to do, if the two vector types aren't
> really compatible one will get an error.
> But then, for trunk, won't the stripping of the attributes from vector types
> still mean that comp_type_attributes will return false when comparing the
> destination vector type (the uint8x16_t one) with the one from the binary
> expression (the same with stripped attribute, i.e. essentially a generic
> vector) and thus the initialization will be considered erroneous?

The initialization itself seems to work, since for vectors
compatibility depends on vector_types_compatible_p.

But the stripping of the attributes does still affect:

#include <arm_neon.h>

uint8x16_t
foo (int c, uint8x16_t x, uint8x16_t y)
{
  return c ? x + 1 : y;
}

which is wrongly rejected for C, but not C++.  So I guess for trunk
we need both fixes.
Comment 9 Richard Sandiford 2020-07-30 17:08:28 UTC
Created attachment 48964 [details]
Patch for the initialisation problem

I'm testing this patch to fix the initialisation side of the problem.
Comment 10 jsm-csl@polyomino.org.uk 2020-07-30 19:55:46 UTC
On Thu, 30 Jul 2020, rsandifo at gcc dot gnu.org wrote:

> IMO process_init_element shouldn't recurse into vector types
> if the initialisation value is also a vector type.  We should
> treat the vector value as initialising the whole type and report
> an error if they're not compatible.

Yes, that seems correct.
Comment 11 GCC Commits 2020-08-01 11:42:01 UTC
The master branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:7d599ad27b9bcf5165f87710f1abc64bbabd06ae

commit r11-2481-g7d599ad27b9bcf5165f87710f1abc64bbabd06ae
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Sat Aug 1 12:41:28 2020 +0100

    c: Fix bogus vector initialisation error [PR96377]
    
    One of the problems in this PR was that if we had:
    
      vector_type1 array[] = { vector_value1 };
    
    process_init_element would only treat vector_value1 as initialising
    a vector_type1 if they had the same TYPE_MAIN_VARIANT.  This has
    several problems:
    
    (1) It gives confusing error messages if the vector types are
        incompatible.  (Tested by gcc.dg/pr96377-1.c.)
    
    (2) It means that we reject code that should be valid with
        -flax-vector-conversions.  (Tested by gcc.dg/pr96377-2.c.)
    
    (3) On arm and aarch64 targets, it means that we reject some
        initializers that mix Advanced SIMD and standard GNU vectors.
        These vectors have traditionally had different TYPE_MAIN_VARIANTs
        because they have different mangling schemes.  (Tested by
        gcc.dg/pr96377-[3-6].c.)
    
    (4) It means that we reject SVE initializers that should be valid.
        (Tested by gcc.target/aarch64/sve/gnu_vectors_[34].c.)
    
    (5) After r11-1741-g:31427b974ed7b7dd54e2 we reject:
    
          arm_neon_type1 array[] = { k ^ arm_neon_value1 };
    
        because applying the binary operator to arm_neon_value1 strips
        the "Advanced SIMD type" attributes that were added in that patch.
        Stripping the attributes is problematic for other reasons though,
        so that still needs to be fixed separately.
    
    g++.target/aarch64/sve/gnu_vectors_[34].C already pass.
    
    gcc/c/
            PR c/96377
            * c-typeck.c (process_init_element): Split test for whether to
            recurse into a record, union or array into...
            (initialize_elementwise_p): ...this new function.  Don't recurse
            into a vector type if the initialization value is also a vector.
    
    gcc/testsuite/
            PR c/96377
            * gcc.dg/pr96377-1.c: New test.
            * gcc.dg/pr96377-2.c: Likewise.
            * gcc.dg/pr96377-3.c: Likewise.
            * gcc.dg/pr96377-4.c: Likewise.
            * gcc.dg/pr96377-5.c: Likewise.
            * gcc.dg/pr96377-6.c: Likewise.
            * gcc.target/aarch64/pr96377-1.c: Likewise.
            * gcc.target/aarch64/sve/acle/general-c/gnu_vectors_3.c: Likewise.
            * gcc.target/aarch64/sve/acle/general-c/gnu_vectors_4.c: Likewise.
            * g++.target/aarch64/sve/acle/general-c++/gnu_vectors_3.C: Likewise.
            * g++.target/aarch64/sve/acle/general-c++/gnu_vectors_4.C: Likewise.
Comment 12 GCC Commits 2020-08-03 08:49:08 UTC
The releases/gcc-10 branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:a216daaa30bc8949086a16e7656f2025b692d03c

commit r10-8562-ga216daaa30bc8949086a16e7656f2025b692d03c
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Mon Aug 3 09:48:36 2020 +0100

    c: Fix bogus vector initialisation error [PR96377]
    
    One of the problems in this PR was that if we had:
    
      vector_type1 array[] = { vector_value1 };
    
    process_init_element would only treat vector_value1 as initialising
    a vector_type1 if they had the same TYPE_MAIN_VARIANT.  This has
    several problems:
    
    (1) It gives confusing error messages if the vector types are
        incompatible.  (Tested by gcc.dg/pr96377-1.c.)
    
    (2) It means that we reject code that should be valid with
        -flax-vector-conversions.  (Tested by gcc.dg/pr96377-2.c.)
    
    (3) On arm and aarch64 targets, it means that we reject some
        initializers that mix Advanced SIMD and standard GNU vectors.
        These vectors have traditionally had different TYPE_MAIN_VARIANTs
        because they have different mangling schemes.  (Tested by
        gcc.dg/pr96377-[3-6].c.)
    
    (4) It means that we reject SVE initializers that should be valid.
        (Tested by gcc.target/aarch64/sve/gnu_vectors_[34].c.)
    
    (5) After r11-1741-g:31427b974ed7b7dd54e2 we reject:
    
          arm_neon_type1 array[] = { k ^ arm_neon_value1 };
    
        because applying the binary operator to arm_neon_value1 strips
        the "Advanced SIMD type" attributes that were added in that patch.
        Stripping the attributes is problematic for other reasons though,
        so that still needs to be fixed separately.
    
    g++.target/aarch64/sve/gnu_vectors_[34].C already pass.
    
    gcc/c/
            PR c/96377
            * c-typeck.c (process_init_element): Split test for whether to
            recurse into a record, union or array into...
            (initialize_elementwise_p): ...this new function.  Don't recurse
            into a vector type if the initialization value is also a vector.
    
    gcc/testsuite/
            PR c/96377
            * gcc.dg/pr96377-1.c: New test.
            * gcc.dg/pr96377-2.c: Likewise.
            * gcc.dg/pr96377-3.c: Likewise.
            * gcc.dg/pr96377-4.c: Likewise.
            * gcc.dg/pr96377-5.c: Likewise.
            * gcc.dg/pr96377-6.c: Likewise.
            * gcc.target/aarch64/pr96377-1.c: Likewise.
            * gcc.target/aarch64/sve/acle/general-c/gnu_vectors_3.c: Likewise.
            * gcc.target/aarch64/sve/acle/general-c/gnu_vectors_4.c: Likewise.
            * g++.target/aarch64/sve/acle/general-c++/gnu_vectors_3.C: Likewise.
            * g++.target/aarch64/sve/acle/general-c++/gnu_vectors_4.C: Likewise.
    
    (cherry picked from commit 7d599ad27b9bcf5165f87710f1abc64bbabd06ae)
Comment 13 Richard Sandiford 2020-09-10 17:21:17 UTC
*** Bug 96990 has been marked as a duplicate of this bug. ***
Comment 14 Jakub Jelinek 2020-11-18 14:33:22 UTC
Fixed.
Comment 15 Richard Sandiford 2020-11-18 16:56:38 UTC
The original problem is fixed for GCC 10 and the kernel
manifestation is fixed for trunk.  We still need to deal
with the testcase in comment 8, which is another manifestation
that affects trunk only.
Comment 16 Richard Biener 2021-01-27 09:28:59 UTC
Please open a new bug for the remaining issue since this one was P1 blocking a 10.3 release.