[Bug target/95523] New: aarch64:ICE in register_tuple_type，at config/aarch64/aarch64-sve-builtins.cc:3434

[Bug target/95523] New: aarch64:ICE in register_tuple_type，at config/aarch64/aarch64-sve-builtins.cc:3434

[z.zhanghaijian at huawei dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95523

            Bug ID: 95523
           Summary: aarch64:ICE in register_tuple_type，at
                    config/aarch64/aarch64-sve-builtins.cc:3434
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: z.zhanghaijian at huawei dot com
  Target Milestone: ---

ICE issue triggered under option -fpack-struct=n:

Example:
test.c：
#include "arm_sve.h"

gcc test.c -S -fpack-struct=2

In file included from test.c:1:
lib/gcc/aarch64-linux-gnu/11.0.0/include/arm_sve.h:40:9: internal compiler
error: in register_tuple_type, at config/aarch64/aarch64-sve-builtins.cc:3434
   40 | #pragma GCC aarch64 "arm_sve.h"
      |         ^~~
0x17ef8b3 register_tuple_type
        ../.././gcc/config/aarch64/aarch64-sve-builtins.cc:3434
0x17f00ff aarch64_sve::handle_arm_sve_h()
        ../.././gcc/config/aarch64/aarch64-sve-builtins.cc:3516
0xae927f aarch64_pragma_aarch64
        ../.././gcc/config/aarch64/aarch64-c.c:281
0xaafe53 c_invoke_pragma_handler(unsigned int)
        ../.././gcc/c-family/c-pragma.c:1501
0x9f6133 c_parser_pragma
        ../.././gcc/c/c-parser.c:12509
0x9dc03f c_parser_external_declaration
        ../.././gcc/c/c-parser.c:1726
0x9dbb43 c_parser_translation_unit
        ../.././gcc/c/c-parser.c:1618
0xa14a23 c_parse_file()
        ../.././gcc/c/c-parser.c:21746
0xaa7ed3 c_common_parse_file()
        ../.././gcc/c-family/c-opts.c:1190

The #pragma GCC aarch64 "arm_sve.h" will tell GCC to insert the necessary type
and function definitions.
When register the tuple type, the function register_tuple_type will check
TYPE_ALIGN (tuple_type) == 128. The option -fpack-struct=n will change the
alignment of the tuple. When -fpack-struct=2, the value of TYPE_ALIGN
(tuple_type) is 16, which will cause ICE.

I think there is no need to judge TYPE_ALIGN, even if type_mode is SVE vector.
Any questions?

Proposed patch:
diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc
b/gcc/config/aarch64/aarch64-sve-builtins.cc
index bdb04e8170d..5bc5af91016 100644
--- a/gcc/config/aarch64/aarch64-sve-builtins.cc
+++ b/gcc/config/aarch64/aarch64-sve-builtins.cc
@@ -3432,8 +3432,7 @@ register_tuple_type (unsigned int num_vectors,
vector_type_index type)
   make_type_sizeless (tuple_type);
   layout_type (tuple_type);
   gcc_assert (VECTOR_MODE_P (TYPE_MODE (tuple_type))
-             && TYPE_MODE_RAW (tuple_type) == TYPE_MODE (tuple_type)
-             && TYPE_ALIGN (tuple_type) == 128);
+             && TYPE_MODE_RAW (tuple_type) == TYPE_MODE (tuple_type));

   /* Work out the structure name.  */
   char buffer[sizeof ("svbfloat16x4_t")];

[Bug target/95523] aarch64:ICE in register_tuple_type，at config/aarch64/aarch64-sve-builtins.cc:3434

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95523

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rsandifo at gcc dot gnu.org
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2020-06-04
     Ever confirmed|0                           |1

--- Comment #1 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
The reason for the assert is that the alignment is part of the
ABI of the types and is relied on when using LDR and STR for
some moves.  Even though -fpack-struct=N changes the ABI in general,
I don't think it should change it in this particular case.

I have to wonder why GCC even has -fpack-struct= though.  Do you have
a specific need for it, or was this caught by option coverage testing?

If there is no specific need, I'd be tempted to make -fpack-struct
unsupported for AArch64.  I think there are so many other things
that could go wrong if it is used.

[Bug target/95523] aarch64:ICE in register_tuple_type，at config/aarch64/aarch64-sve-builtins.cc:3434

[z.zhanghaijian at huawei dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95523

--- Comment #2 from z.zhanghaijian at huawei dot com <z.zhanghaijian at huawei dot com> ---
(In reply to rsandifo@gcc.gnu.org from comment #1)
> The reason for the assert is that the alignment is part of the
> ABI of the types and is relied on when using LDR and STR for
> some moves.  Even though -fpack-struct=N changes the ABI in general,
> I don't think it should change it in this particular case.
> 
> I have to wonder why GCC even has -fpack-struct= though.  Do you have
> a specific need for it, or was this caught by option coverage testing?
> 
> If there is no specific need, I'd be tempted to make -fpack-struct
> unsupported for AArch64.  I think there are so many other things
> that could go wrong if it is used.

We use the option in some projects. If this option is not supported, the impact
is relatively large. I think it can make the error reporting more friendly, so
that this option can still be used normally without sve.

like:
diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc
b/gcc/config/aarch64/aarch64-sve-builtins.cc
index bdb04e8170d..e93e766aba6 100644
--- a/gcc/config/aarch64/aarch64-sve-builtins.cc
+++ b/gcc/config/aarch64/aarch64-sve-builtins.cc
@@ -3504,6 +3504,12 @@ handle_arm_sve_h ()
       return;
     }

+  if (maximum_field_alignment)
+    {
+      error ("SVE is incompatible with the use of %qs or %qs",
"-fpack-struct", "#pragma pack");
+      return;
+    }
+
   sve_switcher sve;

   /* Define the vector and tuple types.  */

Any suggestions?

[Bug target/95523] aarch64:ICE in register_tuple_type，at config/aarch64/aarch64-sve-builtins.cc:3434

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95523

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |NEW

--- Comment #3 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
(In reply to z.zhanghaijian@huawei.com from comment #2)
> (In reply to rsandifo@gcc.gnu.org from comment #1)
> > The reason for the assert is that the alignment is part of the
> > ABI of the types and is relied on when using LDR and STR for
> > some moves.  Even though -fpack-struct=N changes the ABI in general,
> > I don't think it should change it in this particular case.
> > 
> > I have to wonder why GCC even has -fpack-struct= though.  Do you have
> > a specific need for it, or was this caught by option coverage testing?
> > 
> > If there is no specific need, I'd be tempted to make -fpack-struct
> > unsupported for AArch64.  I think there are so many other things
> > that could go wrong if it is used.
> 
> We use the option in some projects. If this option is not supported, the
> impact is relatively large.

Ah, OK.  If it's an issue that affects real use-cases then
let's make it work.

> I think it can make the error reporting more
> friendly, so that this option can still be used normally without sve.
> 
> like:
> diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc
> b/gcc/config/aarch64/aarch64-sve-builtins.cc
> index bdb04e8170d..e93e766aba6 100644
> --- a/gcc/config/aarch64/aarch64-sve-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-sve-builtins.cc
> @@ -3504,6 +3504,12 @@ handle_arm_sve_h ()
>        return;
>      }
> 
> +  if (maximum_field_alignment)
> +    {
> +      error ("SVE is incompatible with the use of %qs or %qs",
> "-fpack-struct", "#pragma pack");
> +      return;
> +    }
> +
>    sve_switcher sve;
> 
>    /* Define the vector and tuple types.  */
> 
> Any suggestions?

Could you try setting DECL_USER_ALIGN on the FIELD_DECL?
that should (hopefully) force the field to keep its
natural alignment.

[Bug target/95523] aarch64:ICE in register_tuple_type，at config/aarch64/aarch64-sve-builtins.cc:3434

[z.zhanghaijian at huawei dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95523

--- Comment #4 from z.zhanghaijian at huawei dot com <z.zhanghaijian at huawei dot com> ---
> Could you try setting DECL_USER_ALIGN on the FIELD_DECL?
> that should (hopefully) force the field to keep its
> natural alignment.

Do you mean to change the alignment to natural alignment when processing
arm_sve.h, and then change it back after handle_arm_sve_h?

I tracked the calculation process of TYPE_ALIGN (tuple_type), which is
determined by maximum_field_alignment in layout_decl, not from DECL_USER_ALIGN.

We can set maximum_field_alignment in sve_switcher to achieve the purpose of
natural alignment.

like:
diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc
b/gcc/config/aarch64/aarch64-sve-builtins.cc
index bdb04e8170d..c49fcebcd43 100644
--- a/gcc/config/aarch64/aarch64-sve-builtins.cc
+++ b/gcc/config/aarch64/aarch64-sve-builtins.cc
@@ -878,6 +878,9 @@ sve_switcher::sve_switcher ()
   aarch64_isa_flags = (AARCH64_FL_FP | AARCH64_FL_SIMD | AARCH64_FL_F16
                       | AARCH64_FL_SVE);

+  m_old_maximum_field_alignment = maximum_field_alignment;
+  maximum_field_alignment = 0;
+
   m_old_general_regs_only = TARGET_GENERAL_REGS_ONLY;
   global_options.x_target_flags &= ~MASK_GENERAL_REGS_ONLY;

@@ -895,6 +898,7 @@ sve_switcher::~sve_switcher ()
   if (m_old_general_regs_only)
     global_options.x_target_flags |= MASK_GENERAL_REGS_ONLY;
   aarch64_isa_flags = m_old_isa_flags;
+  maximum_field_alignment = m_old_maximum_field_alignment;
 }

 function_builder::function_builder ()
diff --git a/gcc/config/aarch64/aarch64-sve-builtins.h
b/gcc/config/aarch64/aarch64-sve-builtins.h
index 526d9f55e7b..3ffe2516df9 100644
--- a/gcc/config/aarch64/aarch64-sve-builtins.h
+++ b/gcc/config/aarch64/aarch64-sve-builtins.h
@@ -658,6 +658,7 @@ public:

 private:
   unsigned long m_old_isa_flags;
+  unsigned int m_old_maximum_field_alignment;
   bool m_old_general_regs_only;
   bool m_old_have_regs_of_mode[MAX_MACHINE_MODE];
 };

[Bug target/95523] aarch64:ICE in register_tuple_type，at config/aarch64/aarch64-sve-builtins.cc:3434

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95523

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>:

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

commit r11-1179-gb5cebc9ab7f6ab47067dc04cae17bf9921a62a18
Author: z00219097 <z.zhanghaijian@huawei.com>
Date:   Wed Jun 10 16:58:51 2020 +0100

    aarch64: Fix an ICE in register_tuple_type [PR95523]

    When registering the tuple type in register_tuple_type, the
    TYPE_ALIGN (tuple_type) will be changed by -fpack-struct=n. We need to
    maintain natural alignment in handle_arm_sve_h.

    2020-06-10  Haijian Zhang  <z.zhanghaijian@huawei.com>

    gcc/
            PR target/95523
            * config/aarch64/aarch64-sve-builtins.h
            (sve_switcher::m_old_maximum_field_alignment): New member.
            * config/aarch64/aarch64-sve-builtins.cc
            (sve_switcher::sve_switcher): Save maximum_field_alignment in
            m_old_maximum_field_alignment and clear maximum_field_alignment.
            (sve_switcher::~sve_switcher): Restore maximum_field_alignment.

    gcc/testsuite/
            PR target/95523
            * gcc.target/aarch64/sve/pr95523.c: New test.

[Bug target/95523] aarch64:ICE in register_tuple_type，at config/aarch64/aarch64-sve-builtins.cc:3434

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95523

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #6 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Fixed.