diff mbox series

Sanitizing the middle-end interface to the back-end for strict alignment

Message ID AM6PR10MB2566CF3F93327C1666133CF4E4AC0@AM6PR10MB2566.EURPRD10.PROD.OUTLOOK.COM
State New
Headers show
Series Sanitizing the middle-end interface to the back-end for strict alignment | expand

Commit Message

Bernd Edlinger Aug. 15, 2019, 7:47 p.m. UTC 
Hi,

this is the split out part from the "Fix not 8-byte aligned ldrd/strd on ARMv5 (PR 89544)"
which is sanitizing the middle-end interface to the back-end for strict alignment,
and a couple of bug-fixes that are necessary to survive boot-strap.
It is intended to be applied after the PR 89544 fix.

I think it would be possible to change the default implementation of STACK_SLOT_ALIGNMENT
to make all stack variables always naturally aligned instead of doing that only
in assign_parm_setup_stack, but would still like to avoid changing too many things
that do not seem to have a problem.  Since this would affect many targets, and more
kinds of variables that may probably not have a strict alignment problem.
But I am ready to take your advice though.


Boot-strapped and reg-tested on x86_64-pc-linux-gnu and arm-linux-gnueabihf
Is it OK for trunk?


Thanks
Bernd.

Comments

Bernd Edlinger Aug. 17, 2019, 7:44 a.m. UTC | #1 
On 8/15/19 9:47 PM, Bernd Edlinger wrote:
> Hi,
> 
> this is the split out part from the "Fix not 8-byte aligned ldrd/strd on ARMv5 (PR 89544)"
> which is sanitizing the middle-end interface to the back-end for strict alignment,
> and a couple of bug-fixes that are necessary to survive boot-strap.
> It is intended to be applied after the PR 89544 fix.
> 
> I think it would be possible to change the default implementation of STACK_SLOT_ALIGNMENT
> to make all stack variables always naturally aligned instead of doing that only
> in assign_parm_setup_stack, but would still like to avoid changing too many things
> that do not seem to have a problem.  Since this would affect many targets, and more
> kinds of variables that may probably not have a strict alignment problem.
> But I am ready to take your advice though.
> 
> 
> Boot-strapped and reg-tested on x86_64-pc-linux-gnu and arm-linux-gnueabihf
> Is it OK for trunk?
> 
> 

Hmm, actually the hunk in assign_parm_setup_stack is not only failing
an assertion, but rather a wrong code bug:

I found now a test case that generates silently wrong code and is fixed
by this patch.

$ cat unaligned-argument-3.c 
/* { dg-do compile } */
/* { dg-require-effective-target arm_arm_ok } */
/* { dg-options "-marm -mno-unaligned-access -O3" } */

typedef int __attribute__((aligned(1))) s;

void x(char*, s*);
void f(char a, s f)
{
  x(&a, &f);
}

/* { dg-final { scan-assembler-times "str\t\[^\\n\]*\\\[sp\\\]" 1 } } */
/* { dg-final { scan-assembler-times "str\t\[^\\n\]*\\\[sp, #3\\\]" 0 } } */

currently with -marm -mno-unaligned-access -O3 we generate:

f:
	@ args = 0, pretend = 0, frame = 8
	@ frame_needed = 0, uses_anonymous_args = 0
	str	lr, [sp, #-4]!
	sub	sp, sp, #12
	mov	r3, r0
	str	r1, [sp, #3]  <- may trap
	add	r0, sp, #7
	add	r1, sp, #3
	strb	r3, [sp, #7]
	bl	x
	add	sp, sp, #12
	@ sp needed
	ldr	pc, [sp], #4


So I would like to add a test case to the patch as attached.

Tested with a cross, that both dg-final fail currently and are fixed
with the other patches applied.

Is it OK for trunk?


Thanks
Bernd.
Jeff Law Aug. 22, 2019, 10:55 p.m. UTC | #2 
On 8/17/19 1:44 AM, Bernd Edlinger wrote:
> On 8/15/19 9:47 PM, Bernd Edlinger wrote:
>> Hi,
>>
>> this is the split out part from the "Fix not 8-byte aligned ldrd/strd on ARMv5 (PR 89544)"
>> which is sanitizing the middle-end interface to the back-end for strict alignment,
>> and a couple of bug-fixes that are necessary to survive boot-strap.
>> It is intended to be applied after the PR 89544 fix.
>>
>> I think it would be possible to change the default implementation of STACK_SLOT_ALIGNMENT
>> to make all stack variables always naturally aligned instead of doing that only
>> in assign_parm_setup_stack, but would still like to avoid changing too many things
>> that do not seem to have a problem.  Since this would affect many targets, and more
>> kinds of variables that may probably not have a strict alignment problem.
>> But I am ready to take your advice though.
>>
>>
>> Boot-strapped and reg-tested on x86_64-pc-linux-gnu and arm-linux-gnueabihf
>> Is it OK for trunk?
>>
>>
> 
> Hmm, actually the hunk in assign_parm_setup_stack is not only failing
> an assertion, but rather a wrong code bug:
> 
> I found now a test case that generates silently wrong code and is fixed
> by this patch.
> 
> $ cat unaligned-argument-3.c 
> /* { dg-do compile } */
> /* { dg-require-effective-target arm_arm_ok } */
> /* { dg-options "-marm -mno-unaligned-access -O3" } */
> 
> typedef int __attribute__((aligned(1))) s;
> 
> void x(char*, s*);
> void f(char a, s f)
> {
>   x(&a, &f);
> }
> 
> /* { dg-final { scan-assembler-times "str\t\[^\\n\]*\\\[sp\\\]" 1 } } */
> /* { dg-final { scan-assembler-times "str\t\[^\\n\]*\\\[sp, #3\\\]" 0 } } */
> 
> currently with -marm -mno-unaligned-access -O3 we generate:
> 
> f:
> 	@ args = 0, pretend = 0, frame = 8
> 	@ frame_needed = 0, uses_anonymous_args = 0
> 	str	lr, [sp, #-4]!
> 	sub	sp, sp, #12
> 	mov	r3, r0
> 	str	r1, [sp, #3]  <- may trap
> 	add	r0, sp, #7
> 	add	r1, sp, #3
> 	strb	r3, [sp, #7]
> 	bl	x
> 	add	sp, sp, #12
> 	@ sp needed
> 	ldr	pc, [sp], #4
> 
> 
> So I would like to add a test case to the patch as attached.
> 
> Tested with a cross, that both dg-final fail currently and are fixed
> with the other patches applied.
> 
> Is it OK for trunk?
OK when the patch that fixes this is ACK'd.

jeff
Jeff Law Aug. 22, 2019, 10:57 p.m. UTC | #3 
On 8/15/19 1:47 PM, Bernd Edlinger wrote:
> Hi,
> 
> this is the split out part from the "Fix not 8-byte aligned ldrd/strd on ARMv5 (PR 89544)"
> which is sanitizing the middle-end interface to the back-end for strict alignment,
> and a couple of bug-fixes that are necessary to survive boot-strap.
> It is intended to be applied after the PR 89544 fix.
> 
> I think it would be possible to change the default implementation of STACK_SLOT_ALIGNMENT
> to make all stack variables always naturally aligned instead of doing that only
> in assign_parm_setup_stack, but would still like to avoid changing too many things
> that do not seem to have a problem.  Since this would affect many targets, and more
> kinds of variables that may probably not have a strict alignment problem.
> But I am ready to take your advice though.
> 
> 
> Boot-strapped and reg-tested on x86_64-pc-linux-gnu and arm-linux-gnueabihf
> Is it OK for trunk?
> 
> 
> Thanks
> Bernd.
> 
> 
> patch-strict-align.diff
> 
> 2019-08-15  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 	    Richard Biener  <rguenther@suse.de>
> 
> 	* expr.c (expand_assignment): Handle misaligned DECLs.
> 	(expand_expr_real_1): Handle FUNCTION_DECL as unaligned.
> 	* function.c (assign_parm_adjust_stack_rtl): Check movmisalign optab
> 	too.
> 	(assign_parm_setup_stack): Allocate properly aligned stack slots.
> 	* varasm.c (build_constant_desc): Align constants of misaligned types.
> 	* config/arm/arm.md (movdi, movsi, movhi, movhf, movsf, movdf): Check
> 	strict alignment restrictions on memory addresses.
> 	* config/arm/neon.md (movti, mov<VSTRUCT>, mov<VH>): Likewise.
> 	* config/arm/vec-common.md (mov<VALL>): Likewise.
I'll ack the generic bits.  I have no clue if the ARM maintainers want
the asserts or not.

jeff
Bernd Edlinger Aug. 23, 2019, 2:34 p.m. UTC | #4 
On 8/23/19 12:57 AM, Jeff Law wrote:
> On 8/15/19 1:47 PM, Bernd Edlinger wrote:
>> Hi,
>>
>> this is the split out part from the "Fix not 8-byte aligned ldrd/strd on ARMv5 (PR 89544)"
>> which is sanitizing the middle-end interface to the back-end for strict alignment,
>> and a couple of bug-fixes that are necessary to survive boot-strap.
>> It is intended to be applied after the PR 89544 fix.
>>
>> I think it would be possible to change the default implementation of STACK_SLOT_ALIGNMENT
>> to make all stack variables always naturally aligned instead of doing that only
>> in assign_parm_setup_stack, but would still like to avoid changing too many things
>> that do not seem to have a problem.  Since this would affect many targets, and more
>> kinds of variables that may probably not have a strict alignment problem.
>> But I am ready to take your advice though.
>>
>>
>> Boot-strapped and reg-tested on x86_64-pc-linux-gnu and arm-linux-gnueabihf
>> Is it OK for trunk?
>>
>>
>> Thanks
>> Bernd.
>>
>>
>> patch-strict-align.diff
>>
>> 2019-08-15  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>> 	    Richard Biener  <rguenther@suse.de>
>>
>> 	* expr.c (expand_assignment): Handle misaligned DECLs.
>> 	(expand_expr_real_1): Handle FUNCTION_DECL as unaligned.
>> 	* function.c (assign_parm_adjust_stack_rtl): Check movmisalign optab
>> 	too.
>> 	(assign_parm_setup_stack): Allocate properly aligned stack slots.
>> 	* varasm.c (build_constant_desc): Align constants of misaligned types.
>> 	* config/arm/arm.md (movdi, movsi, movhi, movhf, movsf, movdf): Check
>> 	strict alignment restrictions on memory addresses.
>> 	* config/arm/neon.md (movti, mov<VSTRUCT>, mov<VH>): Likewise.
>> 	* config/arm/vec-common.md (mov<VALL>): Likewise.
> I'll ack the generic bits.  I have no clue if the ARM maintainers want
> the asserts or not.
> 

Okay, thanks Jeff, and Richi.

So I would like to ping on the ARM platform bits.
Just a couple of gcc_checking_asserts.
The wrong code will be fixed by the middle-end changes alone,
but the assertions would help prevent further wrong-code issues
going unnoticed.

Is it OK for trunk?


Thanks
Bernd.
Kyrill Tkachov Aug. 27, 2019, 9:25 a.m. UTC | #5 
Hi Bernd,

On 8/15/19 8:47 PM, Bernd Edlinger wrote:
> Hi,
>
> this is the split out part from the "Fix not 8-byte aligned ldrd/strd on ARMv5 (PR 89544)"
> which is sanitizing the middle-end interface to the back-end for strict alignment,
> and a couple of bug-fixes that are necessary to survive boot-strap.
> It is intended to be applied after the PR 89544 fix.
>
> I think it would be possible to change the default implementation of STACK_SLOT_ALIGNMENT
> to make all stack variables always naturally aligned instead of doing that only
> in assign_parm_setup_stack, but would still like to avoid changing too many things
> that do not seem to have a problem.  Since this would affect many targets, and more
> kinds of variables that may probably not have a strict alignment problem.
> But I am ready to take your advice though.
>
>
> Boot-strapped and reg-tested on x86_64-pc-linux-gnu and arm-linux-gnueabihf
> Is it OK for trunk?

I'm not opposed to the checks but...


>
> Thanks
> Bernd.
>

Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md	(Revision 274531)
+++ gcc/config/arm/arm.md	(Arbeitskopie)
@@ -5838,6 +5838,12 @@
  	(match_operand:DI 1 "general_operand"))]
    "TARGET_EITHER"
    "
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (DImode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (DImode));
    if (can_create_pseudo_p ())
      {
        if (!REG_P (operands[0]))
@@ -6014,6 +6020,12 @@
    {
    rtx base, offset, tmp;
  
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (SImode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (SImode));
    if (TARGET_32BIT || TARGET_HAVE_MOVT)
      {
        /* Everything except mem = const or mem = mem can be done easily.  */
@@ -6503,6 +6515,12 @@
  	(match_operand:HI 1 "general_operand"))]
    "TARGET_EITHER"
    "
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (HImode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (HImode));
    if (TARGET_ARM)
      {
        if (can_create_pseudo_p ())
@@ -6912,6 +6930,12 @@
  	(match_operand:HF 1 "general_operand"))]
    "TARGET_EITHER"
    "
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (HFmode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (HFmode));
    if (TARGET_32BIT)
      {
        if (MEM_P (operands[0]))
@@ -6976,6 +7000,12 @@
  	(match_operand:SF 1 "general_operand"))]
    "TARGET_EITHER"
    "
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (SFmode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (SFmode));
    if (TARGET_32BIT)
      {
        if (MEM_P (operands[0]))
@@ -7071,6 +7101,12 @@
  	(match_operand:DF 1 "general_operand"))]
    "TARGET_EITHER"
    "
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (DFmode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (DFmode));
    if (TARGET_32BIT)
      {
        if (MEM_P (operands[0]))
Index: gcc/config/arm/neon.md
===================================================================
--- gcc/config/arm/neon.md	(Revision 274531)
+++ gcc/config/arm/neon.md	(Arbeitskopie)
@@ -127,6 +127,12 @@
  	(match_operand:TI 1 "general_operand"))]
    "TARGET_NEON"
  {
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (TImode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (TImode));
    if (can_create_pseudo_p ())
      {
        if (!REG_P (operands[0]))
@@ -139,6 +145,12 @@
  	(match_operand:VSTRUCT 1 "general_operand"))]
    "TARGET_NEON"
  {
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (<MODE>mode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (<MODE>mode));
    if (can_create_pseudo_p ())
      {
        if (!REG_P (operands[0]))
@@ -151,6 +163,12 @@
  	(match_operand:VH 1 "s_register_operand"))]
    "TARGET_NEON"
  {
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (<MODE>mode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (<MODE>mode));
    if (can_create_pseudo_p ())
      {
        if (!REG_P (operands[0]))
Index: gcc/config/arm/vec-common.md
===================================================================
--- gcc/config/arm/vec-common.md	(Revision 274531)
+++ gcc/config/arm/vec-common.md	(Arbeitskopie)
@@ -26,6 +26,12 @@
    "TARGET_NEON
     || (TARGET_REALLY_IWMMXT && VALID_IWMMXT_REG_MODE (<MODE>mode))"
  {
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (<MODE>mode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (<MODE>mode));
    if (can_create_pseudo_p ())
      {
        if (!REG_P (operands[0]))

... can we please factor the (!MEM_P (operands[0]) || MEM_ALIGN (operands[0]) >= GET_MODE_ALIGNMENT (<MODE>mode)) checks into a common function and use that?

Thanks,
Kyrill
Bernd Edlinger Aug. 28, 2019, 9:38 a.m. UTC | #6 
On 8/27/19 11:25 AM, Kyrill Tkachov wrote:
> Hi Bernd,
> 
> On 8/15/19 8:47 PM, Bernd Edlinger wrote:
>> Hi,
>>
>> this is the split out part from the "Fix not 8-byte aligned ldrd/strd on ARMv5 (PR 89544)"
>> which is sanitizing the middle-end interface to the back-end for strict alignment,
>> and a couple of bug-fixes that are necessary to survive boot-strap.
>> It is intended to be applied after the PR 89544 fix.
>>
>> I think it would be possible to change the default implementation of STACK_SLOT_ALIGNMENT
>> to make all stack variables always naturally aligned instead of doing that only
>> in assign_parm_setup_stack, but would still like to avoid changing too many things
>> that do not seem to have a problem.  Since this would affect many targets, and more
>> kinds of variables that may probably not have a strict alignment problem.
>> But I am ready to take your advice though.
>>
>>
>> Boot-strapped and reg-tested on x86_64-pc-linux-gnu and arm-linux-gnueabihf
>> Is it OK for trunk?
> 
> I'm not opposed to the checks but...
> 
> 
>>
>> Thanks
>> Bernd.
>>
> 
> Index: gcc/config/arm/vec-common.md
> ===================================================================
> --- gcc/config/arm/vec-common.md    (Revision 274531)
> +++ gcc/config/arm/vec-common.md    (Arbeitskopie)
> @@ -26,6 +26,12 @@
>    "TARGET_NEON
>     || (TARGET_REALLY_IWMMXT && VALID_IWMMXT_REG_MODE (<MODE>mode))"
>  {
> +  gcc_checking_assert (!MEM_P (operands[0])
> +               || MEM_ALIGN (operands[0])
> +              >= GET_MODE_ALIGNMENT (<MODE>mode));
> +  gcc_checking_assert (!MEM_P (operands[1])
> +               || MEM_ALIGN (operands[1])
> +              >= GET_MODE_ALIGNMENT (<MODE>mode));
>    if (can_create_pseudo_p ())
>      {
>        if (!REG_P (operands[0]))
> 
> ... can we please factor the (!MEM_P (operands[0]) || MEM_ALIGN (operands[0]) >= GET_MODE_ALIGNMENT (<MODE>mode)) checks into a common function and use that?
> 

Sure, good idea.  How about converting it to a predicate?
This creates 1:1 equivalent code to the open coded assertions.

Is it OK for trunk?


Thanks
Bernd.
Kyrill Tkachov Aug. 28, 2019, 9:42 a.m. UTC | #7 
On 8/28/19 10:38 AM, Bernd Edlinger wrote:
> On 8/27/19 11:25 AM, Kyrill Tkachov wrote:
>> Hi Bernd,
>>
>> On 8/15/19 8:47 PM, Bernd Edlinger wrote:
>>> Hi,
>>>
>>> this is the split out part from the "Fix not 8-byte aligned ldrd/strd on ARMv5 (PR 89544)"
>>> which is sanitizing the middle-end interface to the back-end for strict alignment,
>>> and a couple of bug-fixes that are necessary to survive boot-strap.
>>> It is intended to be applied after the PR 89544 fix.
>>>
>>> I think it would be possible to change the default implementation of STACK_SLOT_ALIGNMENT
>>> to make all stack variables always naturally aligned instead of doing that only
>>> in assign_parm_setup_stack, but would still like to avoid changing too many things
>>> that do not seem to have a problem.  Since this would affect many targets, and more
>>> kinds of variables that may probably not have a strict alignment problem.
>>> But I am ready to take your advice though.
>>>
>>>
>>> Boot-strapped and reg-tested on x86_64-pc-linux-gnu and arm-linux-gnueabihf
>>> Is it OK for trunk?
>> I'm not opposed to the checks but...
>>
>>
>>> Thanks
>>> Bernd.
>>>
>> Index: gcc/config/arm/vec-common.md
>> ===================================================================
>> --- gcc/config/arm/vec-common.md    (Revision 274531)
>> +++ gcc/config/arm/vec-common.md    (Arbeitskopie)
>> @@ -26,6 +26,12 @@
>>     "TARGET_NEON
>>      || (TARGET_REALLY_IWMMXT && VALID_IWMMXT_REG_MODE (<MODE>mode))"
>>   {
>> +  gcc_checking_assert (!MEM_P (operands[0])
>> +               || MEM_ALIGN (operands[0])
>> +              >= GET_MODE_ALIGNMENT (<MODE>mode));
>> +  gcc_checking_assert (!MEM_P (operands[1])
>> +               || MEM_ALIGN (operands[1])
>> +              >= GET_MODE_ALIGNMENT (<MODE>mode));
>>     if (can_create_pseudo_p ())
>>       {
>>         if (!REG_P (operands[0]))
>>
>> ... can we please factor the (!MEM_P (operands[0]) || MEM_ALIGN (operands[0]) >= GET_MODE_ALIGNMENT (<MODE>mode)) checks into a common function and use that?
>>
> Sure, good idea.  How about converting it to a predicate?
> This creates 1:1 equivalent code to the open coded assertions.
>
> Is it OK for trunk?
>
>
> Thanks
> Bernd.


patch-strict-align.diff

2019-08-15  Bernd Edlinger<bernd.edlinger@hotmail.de>
	    Richard Biener<rguenther@suse.de>

	* expr.c (expand_assignment): Handle misaligned DECLs.
	(expand_expr_real_1): Handle FUNCTION_DECL as unaligned.
	* function.c (assign_parm_adjust_stack_rtl): Check movmisalign optab
	too.
	(assign_parm_setup_stack): Allocate properly aligned stack slots.
	* varasm.c (build_constant_desc): Align constants of misaligned types.
	* config/arm/predicates.md (aligned_operand): New predicate.
	* config/arm/arm.md (movdi, movsi, movhi, movhf, movsf, movdf): Use
	sligned_operand to check restrictions on memory addresses.

typo in "aligned_operand"

         * config/arm/neon.md (movti, mov<VSTRUCT>, mov<VH>): Likewise.
	* config/arm/vec-common.md (mov<VALL>): Likewise.


Looks good now.

Ok, thanks!

Kyrill
Christophe Lyon Aug. 28, 2019, 12:07 p.m. UTC | #8 
On Wed, 28 Aug 2019 at 11:42, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
>
>
> On 8/28/19 10:38 AM, Bernd Edlinger wrote:
> > On 8/27/19 11:25 AM, Kyrill Tkachov wrote:
> >> Hi Bernd,
> >>
> >> On 8/15/19 8:47 PM, Bernd Edlinger wrote:
> >>> Hi,
> >>>
> >>> this is the split out part from the "Fix not 8-byte aligned ldrd/strd on ARMv5 (PR 89544)"
> >>> which is sanitizing the middle-end interface to the back-end for strict alignment,
> >>> and a couple of bug-fixes that are necessary to survive boot-strap.
> >>> It is intended to be applied after the PR 89544 fix.
> >>>
> >>> I think it would be possible to change the default implementation of STACK_SLOT_ALIGNMENT
> >>> to make all stack variables always naturally aligned instead of doing that only
> >>> in assign_parm_setup_stack, but would still like to avoid changing too many things
> >>> that do not seem to have a problem.  Since this would affect many targets, and more
> >>> kinds of variables that may probably not have a strict alignment problem.
> >>> But I am ready to take your advice though.
> >>>
> >>>
> >>> Boot-strapped and reg-tested on x86_64-pc-linux-gnu and arm-linux-gnueabihf
> >>> Is it OK for trunk?
> >> I'm not opposed to the checks but...
> >>
> >>
> >>> Thanks
> >>> Bernd.
> >>>
> >> Index: gcc/config/arm/vec-common.md
> >> ===================================================================
> >> --- gcc/config/arm/vec-common.md    (Revision 274531)
> >> +++ gcc/config/arm/vec-common.md    (Arbeitskopie)
> >> @@ -26,6 +26,12 @@
> >>     "TARGET_NEON
> >>      || (TARGET_REALLY_IWMMXT && VALID_IWMMXT_REG_MODE (<MODE>mode))"
> >>   {
> >> +  gcc_checking_assert (!MEM_P (operands[0])
> >> +               || MEM_ALIGN (operands[0])
> >> +              >= GET_MODE_ALIGNMENT (<MODE>mode));
> >> +  gcc_checking_assert (!MEM_P (operands[1])
> >> +               || MEM_ALIGN (operands[1])
> >> +              >= GET_MODE_ALIGNMENT (<MODE>mode));
> >>     if (can_create_pseudo_p ())
> >>       {
> >>         if (!REG_P (operands[0]))
> >>
> >> ... can we please factor the (!MEM_P (operands[0]) || MEM_ALIGN (operands[0]) >= GET_MODE_ALIGNMENT (<MODE>mode)) checks into a common function and use that?
> >>
> > Sure, good idea.  How about converting it to a predicate?
> > This creates 1:1 equivalent code to the open coded assertions.
> >
> > Is it OK for trunk?
> >
> >
> > Thanks
> > Bernd.
>
>
> patch-strict-align.diff
>
> 2019-08-15  Bernd Edlinger<bernd.edlinger@hotmail.de>
>             Richard Biener<rguenther@suse.de>
>
>         * expr.c (expand_assignment): Handle misaligned DECLs.
>         (expand_expr_real_1): Handle FUNCTION_DECL as unaligned.
>         * function.c (assign_parm_adjust_stack_rtl): Check movmisalign optab
>         too.
>         (assign_parm_setup_stack): Allocate properly aligned stack slots.
>         * varasm.c (build_constant_desc): Align constants of misaligned types.
>         * config/arm/predicates.md (aligned_operand): New predicate.
>         * config/arm/arm.md (movdi, movsi, movhi, movhf, movsf, movdf): Use
>         sligned_operand to check restrictions on memory addresses.
>
> typo in "aligned_operand"
>
>          * config/arm/neon.md (movti, mov<VSTRUCT>, mov<VH>): Likewise.
>         * config/arm/vec-common.md (mov<VALL>): Likewise.
>
>
> Looks good now.
>

Hi,

This patch causes an ICE when building libgcc's unwind-arm.o
when configuring GCC:
--target  arm-none-linux-gnueabihf --with-mode thumb --with-cpu
cortex-a15 --with-fpu neon-vfpv4:

The build works for the same target, but --with-mode arm --with-cpu
cortex a9 --with-fpu vfp

In file included from
/tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/config/arm/unwind-arm.c:144:
/tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:
In function 'get_eit_entry':
/tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:245:29:
warning: cast discards 'const' qualifier from pointer target type
[-Wcast-qual]
  245 |       ucbp->pr_cache.ehtp = (_Unwind_EHT_Header *)&eitp->content;
      |                             ^
during RTL pass: expand
/tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:
In function 'unwind_phase2_forced':
/tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:319:18:
internal compiler error: in gen_movdi, at config/arm/arm.md:5235
  319 |   saved_vrs.core = entry_vrs->core;
      |   ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
0x126530f gen_movdi(rtx_def*, rtx_def*)
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.md:5235
0x896d92 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/recog.h:318
0x896d92 emit_move_insn_1(rtx_def*, rtx_def*)
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:3694
0x897083 emit_move_insn(rtx_def*, rtx_def*)
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:3790
0xfc25d6 gen_cpymem_ldrd_strd(rtx_def**)
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.c:14582
0x126a1f1 gen_cpymemqi(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.md:6688
0xb0bc08 maybe_expand_insn(insn_code, unsigned int, expand_operand*)
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/optabs.c:7440
0x89ba1e emit_block_move_via_cpymem
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1808
0x89ba1e emit_block_move_hints(rtx_def*, rtx_def*, rtx_def*,
block_op_methods, unsigned int, long, unsigned long, unsigned long,
unsigned long, bool, bool*)
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1627
0x89c383 emit_block_move(rtx_def*, rtx_def*, rtx_def*, block_op_methods)
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1667
0x89fb4e store_expr(tree_node*, rtx_def*, int, bool, bool)
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:5845
0x88c1f9 store_field
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:7149
0x8a0c22 expand_assignment(tree_node*, tree_node*, bool)
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:5304
0x761964 expand_gimple_stmt_1
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:3779
0x761964 expand_gimple_stmt
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:3875
0x768583 expand_gimple_basic_block
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:5915
0x76abc6 execute
        /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:6538

Christophe

> Ok, thanks!
>
> Kyrill
>
Bernd Edlinger Aug. 28, 2019, 9:36 p.m. UTC | #9 
On 8/28/19 2:07 PM, Christophe Lyon wrote:
> Hi,
> 
> This patch causes an ICE when building libgcc's unwind-arm.o
> when configuring GCC:
> --target  arm-none-linux-gnueabihf --with-mode thumb --with-cpu
> cortex-a15 --with-fpu neon-vfpv4:
> 
> The build works for the same target, but --with-mode arm --with-cpu
> cortex a9 --with-fpu vfp
> 
> In file included from
> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/config/arm/unwind-arm.c:144:
> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:
> In function 'get_eit_entry':
> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:245:29:
> warning: cast discards 'const' qualifier from pointer target type
> [-Wcast-qual]
>   245 |       ucbp->pr_cache.ehtp = (_Unwind_EHT_Header *)&eitp->content;
>       |                             ^
> during RTL pass: expand
> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:
> In function 'unwind_phase2_forced':
> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:319:18:
> internal compiler error: in gen_movdi, at config/arm/arm.md:5235
>   319 |   saved_vrs.core = entry_vrs->core;
>       |   ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
> 0x126530f gen_movdi(rtx_def*, rtx_def*)
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.md:5235
> 0x896d92 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/recog.h:318
> 0x896d92 emit_move_insn_1(rtx_def*, rtx_def*)
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:3694
> 0x897083 emit_move_insn(rtx_def*, rtx_def*)
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:3790
> 0xfc25d6 gen_cpymem_ldrd_strd(rtx_def**)
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.c:14582
> 0x126a1f1 gen_cpymemqi(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.md:6688
> 0xb0bc08 maybe_expand_insn(insn_code, unsigned int, expand_operand*)
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/optabs.c:7440
> 0x89ba1e emit_block_move_via_cpymem
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1808
> 0x89ba1e emit_block_move_hints(rtx_def*, rtx_def*, rtx_def*,
> block_op_methods, unsigned int, long, unsigned long, unsigned long,
> unsigned long, bool, bool*)
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1627
> 0x89c383 emit_block_move(rtx_def*, rtx_def*, rtx_def*, block_op_methods)
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1667
> 0x89fb4e store_expr(tree_node*, rtx_def*, int, bool, bool)
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:5845
> 0x88c1f9 store_field
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:7149
> 0x8a0c22 expand_assignment(tree_node*, tree_node*, bool)
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:5304
> 0x761964 expand_gimple_stmt_1
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:3779
> 0x761964 expand_gimple_stmt
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:3875
> 0x768583 expand_gimple_basic_block
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:5915
> 0x76abc6 execute
>         /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:6538
> 
> Christophe
> 

Okay, sorry for the breakage.

What is happening in gen_cpymem_ldrd_strd is of course against the rules:

It uses emit_move_insn on only 4-byte aligned DI-mode memory operands.

I have a patch for this, which is able to fix the libgcc build on a cross, but have no
possibility to bootstrap the affected target.

Could you please help?


Thanks
Bernd.
Kyrill Tkachov Aug. 29, 2019, 8:58 a.m. UTC | #10 
Hi Bernd,

On 8/28/19 10:36 PM, Bernd Edlinger wrote:
> On 8/28/19 2:07 PM, Christophe Lyon wrote:
>> Hi,
>>
>> This patch causes an ICE when building libgcc's unwind-arm.o
>> when configuring GCC:
>> --target  arm-none-linux-gnueabihf --with-mode thumb --with-cpu
>> cortex-a15 --with-fpu neon-vfpv4:
>>
>> The build works for the same target, but --with-mode arm --with-cpu
>> cortex a9 --with-fpu vfp
>>
>> In file included from
>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/config/arm/unwind-arm.c:144:
>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:
>> In function 'get_eit_entry':
>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:245:29:
>> warning: cast discards 'const' qualifier from pointer target type
>> [-Wcast-qual]
>>    245 |       ucbp->pr_cache.ehtp = (_Unwind_EHT_Header *)&eitp->content;
>>        |                             ^
>> during RTL pass: expand
>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:
>> In function 'unwind_phase2_forced':
>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:319:18:
>> internal compiler error: in gen_movdi, at config/arm/arm.md:5235
>>    319 |   saved_vrs.core = entry_vrs->core;
>>        |   ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
>> 0x126530f gen_movdi(rtx_def*, rtx_def*)
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.md:5235
>> 0x896d92 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/recog.h:318
>> 0x896d92 emit_move_insn_1(rtx_def*, rtx_def*)
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:3694
>> 0x897083 emit_move_insn(rtx_def*, rtx_def*)
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:3790
>> 0xfc25d6 gen_cpymem_ldrd_strd(rtx_def**)
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.c:14582
>> 0x126a1f1 gen_cpymemqi(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.md:6688
>> 0xb0bc08 maybe_expand_insn(insn_code, unsigned int, expand_operand*)
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/optabs.c:7440
>> 0x89ba1e emit_block_move_via_cpymem
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1808
>> 0x89ba1e emit_block_move_hints(rtx_def*, rtx_def*, rtx_def*,
>> block_op_methods, unsigned int, long, unsigned long, unsigned long,
>> unsigned long, bool, bool*)
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1627
>> 0x89c383 emit_block_move(rtx_def*, rtx_def*, rtx_def*, block_op_methods)
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1667
>> 0x89fb4e store_expr(tree_node*, rtx_def*, int, bool, bool)
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:5845
>> 0x88c1f9 store_field
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:7149
>> 0x8a0c22 expand_assignment(tree_node*, tree_node*, bool)
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:5304
>> 0x761964 expand_gimple_stmt_1
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:3779
>> 0x761964 expand_gimple_stmt
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:3875
>> 0x768583 expand_gimple_basic_block
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:5915
>> 0x76abc6 execute
>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:6538
>>
>> Christophe
>>
> Okay, sorry for the breakage.
>
> What is happening in gen_cpymem_ldrd_strd is of course against the rules:
>
> It uses emit_move_insn on only 4-byte aligned DI-mode memory operands.
>
> I have a patch for this, which is able to fix the libgcc build on a cross, but have no
> possibility to bootstrap the affected target.
>
> Could you please help?

Well it's good that the sanitisation is catching the bugs!

Bootstrapping this patch I get another assert with the backtrace:

$BUILD/arm-none-linux-gnueabihf/libstdc++-v3/include/ext/concurrence.h: 
In function '(static initializers for 
$SRC/libstdc++-v3/libsupc++/eh_alloc.cc)':
$BUILD/arm-none-linux-gnueabihf/libstdc++-v3/include/ext/concurrence.h:129:5: 
internal compiler error: in gen_movv8qi, at config/arm/vec-common.md:29
   129 |     {
       |     ^
0x14155cb gen_movv8qi(rtx_def*, rtx_def*)
         $SRC/gcc/config/arm/vec-common.md:29
0x96bb89 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
         $SRC/gcc/recog.h:318
0x94bc95 emit_move_insn_1(rtx_def*, rtx_def*)
         $SRC/gcc/expr.c:3694
0x94c05b emit_move_insn(rtx_def*, rtx_def*)
         $SRC/gcc/expr.c:3790
0x10d5ee5 arm_block_set_aligned_vect
         $SRC/gcc/config/arm/arm.c:30204
0x10d6b37 arm_block_set_vect
         $SRC/gcc/config/arm/arm.c:30428
0x10d6caf arm_gen_setmem(rtx_def**)
         $SRC/gcc/config/arm/arm.c:30458
0x140d7ed gen_setmemsi(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
         $SRC/gcc/config/arm/arm.md:6687
0xbf0e87 insn_gen_fn::operator()(rtx_def*, rtx_def*, rtx_def*, rtx_def*) 
const
         $SRC/gcc/recog.h:320
0xbf0999 maybe_gen_insn(insn_code, unsigned int, expand_operand*)
         $SRC/gcc/optabs.c:7409
0xbf0b87 maybe_expand_insn(insn_code, unsigned int, expand_operand*)
         $SRC/gcc/optabs.c:7440
0x94a709 set_storage_via_setmem(rtx_def*, rtx_def*, rtx_def*, unsigned 
int, unsigned int, long long, unsigned long long, unsigned long long, 
unsigned long long)
         $SRC/gcc/expr.c:3168
0x94a059 clear_storage_hints(rtx_def*, rtx_def*, block_op_methods, 
unsigned int, long long, unsigned long long, unsigned long long, 
unsigned long long)
         $SRC/gcc/expr.c:3037
0x94a137 clear_storage(rtx_def*, rtx_def*, block_op_methods)
         $SRC/gcc/expr.c:3058
0x9537c5 store_constructor
         $SRC/gcc/expr.c:6333
0x957227 store_field
         $SRC/gcc/expr.c:7145
0x94fde1 expand_assignment(tree_node*, tree_node*, bool)
         $SRC/gcc/expr.c:5301
0x815e25 expand_gimple_stmt_1
         $SRC/gcc/cfgexpand.c:3777
0x81611d expand_gimple_stmt
         $SRC/gcc/cfgexpand.c:3875
0x81cd61 expand_gimple_basic_block
         $SRC/gcc/cfgexpand.c:5915

Looks to me like arm_gen_setmem needs similar fixes to gen_cpymem_ldrd_strd?

Thanks,

Kyrill


>
>
> Thanks
> Bernd.
Christophe Lyon Aug. 29, 2019, 9:08 a.m. UTC | #11 
On Thu, 29 Aug 2019 at 10:58, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
>
> Hi Bernd,
>
> On 8/28/19 10:36 PM, Bernd Edlinger wrote:
> > On 8/28/19 2:07 PM, Christophe Lyon wrote:
> >> Hi,
> >>
> >> This patch causes an ICE when building libgcc's unwind-arm.o
> >> when configuring GCC:
> >> --target  arm-none-linux-gnueabihf --with-mode thumb --with-cpu
> >> cortex-a15 --with-fpu neon-vfpv4:
> >>
> >> The build works for the same target, but --with-mode arm --with-cpu
> >> cortex a9 --with-fpu vfp
> >>
> >> In file included from
> >> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/config/arm/unwind-arm.c:144:
> >> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:
> >> In function 'get_eit_entry':
> >> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:245:29:
> >> warning: cast discards 'const' qualifier from pointer target type
> >> [-Wcast-qual]
> >>    245 |       ucbp->pr_cache.ehtp = (_Unwind_EHT_Header *)&eitp->content;
> >>        |                             ^
> >> during RTL pass: expand
> >> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:
> >> In function 'unwind_phase2_forced':
> >> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:319:18:
> >> internal compiler error: in gen_movdi, at config/arm/arm.md:5235
> >>    319 |   saved_vrs.core = entry_vrs->core;
> >>        |   ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
> >> 0x126530f gen_movdi(rtx_def*, rtx_def*)
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.md:5235
> >> 0x896d92 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/recog.h:318
> >> 0x896d92 emit_move_insn_1(rtx_def*, rtx_def*)
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:3694
> >> 0x897083 emit_move_insn(rtx_def*, rtx_def*)
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:3790
> >> 0xfc25d6 gen_cpymem_ldrd_strd(rtx_def**)
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.c:14582
> >> 0x126a1f1 gen_cpymemqi(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.md:6688
> >> 0xb0bc08 maybe_expand_insn(insn_code, unsigned int, expand_operand*)
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/optabs.c:7440
> >> 0x89ba1e emit_block_move_via_cpymem
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1808
> >> 0x89ba1e emit_block_move_hints(rtx_def*, rtx_def*, rtx_def*,
> >> block_op_methods, unsigned int, long, unsigned long, unsigned long,
> >> unsigned long, bool, bool*)
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1627
> >> 0x89c383 emit_block_move(rtx_def*, rtx_def*, rtx_def*, block_op_methods)
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1667
> >> 0x89fb4e store_expr(tree_node*, rtx_def*, int, bool, bool)
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:5845
> >> 0x88c1f9 store_field
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:7149
> >> 0x8a0c22 expand_assignment(tree_node*, tree_node*, bool)
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:5304
> >> 0x761964 expand_gimple_stmt_1
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:3779
> >> 0x761964 expand_gimple_stmt
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:3875
> >> 0x768583 expand_gimple_basic_block
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:5915
> >> 0x76abc6 execute
> >>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:6538
> >>
> >> Christophe
> >>
> > Okay, sorry for the breakage.
> >
> > What is happening in gen_cpymem_ldrd_strd is of course against the rules:
> >
> > It uses emit_move_insn on only 4-byte aligned DI-mode memory operands.
> >
> > I have a patch for this, which is able to fix the libgcc build on a cross, but have no
> > possibility to bootstrap the affected target.
> >
> > Could you please help?
>
> Well it's good that the sanitisation is catching the bugs!
>
> Bootstrapping this patch I get another assert with the backtrace:

Thanks for the additional testing, Kyrill!

FWIW, my original report was with a failure to just build GCC for
cortex-a15. I later got the reports of testing cross-toolchains, and
saw other problems on cortex-a9 for instance.
But I guess, you have noticed them with your bootstrap?
on arm-linux-gnueabi
gcc.target/arm/aapcs/align4.c (internal compiler error)
gcc.target/arm/aapcs/align_rec4.c (internal compiler error)

(with -march=armv5t: gcc.dg/pr83930.c (internal compiler error))

on arm-linux-gnueabihf, in addition to align4/align_rec4:
--with-cpu cortex-a9
--with-fpu neon-fp16
    gcc.c-torture/execute/pr37573.c   -O3 -fomit-frame-pointer
-funroll-loops -fpeel-loops -ftracer -finline-functions  (internal
compiler error)
    gcc.c-torture/execute/pr37573.c   -O3 -g  (internal compiler error)
    gcc.dg/vect/fast-math-pr35982.c (internal compiler error)
    gcc.dg/vect/pr55857-1.c (internal compiler error)
    gcc.dg/vect/pr55857-1.c -flto -ffat-lto-objects (internal compiler error)
    gcc.dg/vect/pr55857-2.c (internal compiler error)
    gcc.dg/vect/pr55857-2.c -flto -ffat-lto-objects (internal compiler error)
    gcc.dg/vect/pr57558-2.c (internal compiler error)
    gcc.dg/vect/pr57558-2.c -flto -ffat-lto-objects (internal compiler error)

and even more with other configs
(http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/274986/report-build-info.html
may help)

Christophe

>
> $BUILD/arm-none-linux-gnueabihf/libstdc++-v3/include/ext/concurrence.h:
> In function '(static initializers for
> $SRC/libstdc++-v3/libsupc++/eh_alloc.cc)':
> $BUILD/arm-none-linux-gnueabihf/libstdc++-v3/include/ext/concurrence.h:129:5:
> internal compiler error: in gen_movv8qi, at config/arm/vec-common.md:29
>    129 |     {
>        |     ^
> 0x14155cb gen_movv8qi(rtx_def*, rtx_def*)
>          $SRC/gcc/config/arm/vec-common.md:29
> 0x96bb89 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
>          $SRC/gcc/recog.h:318
> 0x94bc95 emit_move_insn_1(rtx_def*, rtx_def*)
>          $SRC/gcc/expr.c:3694
> 0x94c05b emit_move_insn(rtx_def*, rtx_def*)
>          $SRC/gcc/expr.c:3790
> 0x10d5ee5 arm_block_set_aligned_vect
>          $SRC/gcc/config/arm/arm.c:30204
> 0x10d6b37 arm_block_set_vect
>          $SRC/gcc/config/arm/arm.c:30428
> 0x10d6caf arm_gen_setmem(rtx_def**)
>          $SRC/gcc/config/arm/arm.c:30458
> 0x140d7ed gen_setmemsi(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
>          $SRC/gcc/config/arm/arm.md:6687
> 0xbf0e87 insn_gen_fn::operator()(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
> const
>          $SRC/gcc/recog.h:320
> 0xbf0999 maybe_gen_insn(insn_code, unsigned int, expand_operand*)
>          $SRC/gcc/optabs.c:7409
> 0xbf0b87 maybe_expand_insn(insn_code, unsigned int, expand_operand*)
>          $SRC/gcc/optabs.c:7440
> 0x94a709 set_storage_via_setmem(rtx_def*, rtx_def*, rtx_def*, unsigned
> int, unsigned int, long long, unsigned long long, unsigned long long,
> unsigned long long)
>          $SRC/gcc/expr.c:3168
> 0x94a059 clear_storage_hints(rtx_def*, rtx_def*, block_op_methods,
> unsigned int, long long, unsigned long long, unsigned long long,
> unsigned long long)
>          $SRC/gcc/expr.c:3037
> 0x94a137 clear_storage(rtx_def*, rtx_def*, block_op_methods)
>          $SRC/gcc/expr.c:3058
> 0x9537c5 store_constructor
>          $SRC/gcc/expr.c:6333
> 0x957227 store_field
>          $SRC/gcc/expr.c:7145
> 0x94fde1 expand_assignment(tree_node*, tree_node*, bool)
>          $SRC/gcc/expr.c:5301
> 0x815e25 expand_gimple_stmt_1
>          $SRC/gcc/cfgexpand.c:3777
> 0x81611d expand_gimple_stmt
>          $SRC/gcc/cfgexpand.c:3875
> 0x81cd61 expand_gimple_basic_block
>          $SRC/gcc/cfgexpand.c:5915
>
> Looks to me like arm_gen_setmem needs similar fixes to gen_cpymem_ldrd_strd?
>
> Thanks,
>
> Kyrill
>
>
> >
> >
> > Thanks
> > Bernd.
Bernd Edlinger Aug. 29, 2019, 9:26 p.m. UTC | #12 
On 8/29/19 11:08 AM, Christophe Lyon wrote:
> On Thu, 29 Aug 2019 at 10:58, Kyrill Tkachov
> <kyrylo.tkachov@foss.arm.com> wrote:
>>
>> Hi Bernd,
>>
>> On 8/28/19 10:36 PM, Bernd Edlinger wrote:
>>> On 8/28/19 2:07 PM, Christophe Lyon wrote:
>>>> Hi,
>>>>
>>>> This patch causes an ICE when building libgcc's unwind-arm.o
>>>> when configuring GCC:
>>>> --target  arm-none-linux-gnueabihf --with-mode thumb --with-cpu
>>>> cortex-a15 --with-fpu neon-vfpv4:
>>>>
>>>> The build works for the same target, but --with-mode arm --with-cpu
>>>> cortex a9 --with-fpu vfp
>>>>
>>>> In file included from
>>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/config/arm/unwind-arm.c:144:
>>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:
>>>> In function 'get_eit_entry':
>>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:245:29:
>>>> warning: cast discards 'const' qualifier from pointer target type
>>>> [-Wcast-qual]
>>>>    245 |       ucbp->pr_cache.ehtp = (_Unwind_EHT_Header *)&eitp->content;
>>>>        |                             ^
>>>> during RTL pass: expand
>>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:
>>>> In function 'unwind_phase2_forced':
>>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:319:18:
>>>> internal compiler error: in gen_movdi, at config/arm/arm.md:5235
>>>>    319 |   saved_vrs.core = entry_vrs->core;
>>>>        |   ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
>>>> 0x126530f gen_movdi(rtx_def*, rtx_def*)
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.md:5235
>>>> 0x896d92 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/recog.h:318
>>>> 0x896d92 emit_move_insn_1(rtx_def*, rtx_def*)
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:3694
>>>> 0x897083 emit_move_insn(rtx_def*, rtx_def*)
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:3790
>>>> 0xfc25d6 gen_cpymem_ldrd_strd(rtx_def**)
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.c:14582
>>>> 0x126a1f1 gen_cpymemqi(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.md:6688
>>>> 0xb0bc08 maybe_expand_insn(insn_code, unsigned int, expand_operand*)
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/optabs.c:7440
>>>> 0x89ba1e emit_block_move_via_cpymem
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1808
>>>> 0x89ba1e emit_block_move_hints(rtx_def*, rtx_def*, rtx_def*,
>>>> block_op_methods, unsigned int, long, unsigned long, unsigned long,
>>>> unsigned long, bool, bool*)
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1627
>>>> 0x89c383 emit_block_move(rtx_def*, rtx_def*, rtx_def*, block_op_methods)
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1667
>>>> 0x89fb4e store_expr(tree_node*, rtx_def*, int, bool, bool)
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:5845
>>>> 0x88c1f9 store_field
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:7149
>>>> 0x8a0c22 expand_assignment(tree_node*, tree_node*, bool)
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:5304
>>>> 0x761964 expand_gimple_stmt_1
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:3779
>>>> 0x761964 expand_gimple_stmt
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:3875
>>>> 0x768583 expand_gimple_basic_block
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:5915
>>>> 0x76abc6 execute
>>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:6538
>>>>
>>>> Christophe
>>>>
>>> Okay, sorry for the breakage.
>>>
>>> What is happening in gen_cpymem_ldrd_strd is of course against the rules:
>>>
>>> It uses emit_move_insn on only 4-byte aligned DI-mode memory operands.
>>>
>>> I have a patch for this, which is able to fix the libgcc build on a cross, but have no
>>> possibility to bootstrap the affected target.
>>>
>>> Could you please help?
>>
>> Well it's good that the sanitisation is catching the bugs!
>>

Yes, more than expected, though ;)

>> Bootstrapping this patch I get another assert with the backtrace:
> 
> Thanks for the additional testing, Kyrill!
> 
> FWIW, my original report was with a failure to just build GCC for
> cortex-a15. I later got the reports of testing cross-toolchains, and
> saw other problems on cortex-a9 for instance.
> But I guess, you have noticed them with your bootstrap?
> on arm-linux-gnueabi
> gcc.target/arm/aapcs/align4.c (internal compiler error)
> gcc.target/arm/aapcs/align_rec4.c (internal compiler error)
> 

This appears to be yet unknown middle-end bug (not fixed by current patch)

$ arm-linux-gnueabihf-gcc align4.c 
during RTL pass: expand
In file included from align4.c:22:
align4.c: In function 'testfunc':
abitest.h:73:42: internal compiler error: in gen_movv2si, at config/arm/vec-common.md:30
   73 | #define LAST_ARG(type,val,offset) { type __x = val; if (memcmp(&__x, stack+offset, sizeof(type)) != 0) abort(); }
      |                                          ^~~
abitest.h:74:30: note: in expansion of macro 'LAST_ARG'
   74 | #define ARG(type,val,offset) LAST_ARG(type, val, offset)
      |                              ^~~~~~~~
align4.c:26:3: note: in expansion of macro 'ARG'
   26 |   ARG (unalignedvec, a, R2)
      |   ^~~
0x7bb33c gen_movv2si(rtx_def*, rtx_def*)
	../../gcc-trunk/gcc/config/arm/vec-common.md:30
0xa4a807 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
	../../gcc-trunk/gcc/recog.h:318
0xa4a807 emit_move_insn_1(rtx_def*, rtx_def*)
	../../gcc-trunk/gcc/expr.c:3694
0xa4ab94 emit_move_insn(rtx_def*, rtx_def*)
	../../gcc-trunk/gcc/expr.c:3790
0xa522bf store_expr(tree_node*, rtx_def*, int, bool, bool)
	../../gcc-trunk/gcc/expr.c:5855
0xa52bfd expand_assignment(tree_node*, tree_node*, bool)
	../../gcc-trunk/gcc/expr.c:5441
0xa52bfd expand_assignment(tree_node*, tree_node*, bool)
	../../gcc-trunk/gcc/expr.c:4982
0x934adf expand_gimple_stmt_1
	../../gcc-trunk/gcc/cfgexpand.c:3777
0x934adf expand_gimple_stmt
	../../gcc-trunk/gcc/cfgexpand.c:3875
0x93a451 expand_gimple_basic_block
	../../gcc-trunk/gcc/cfgexpand.c:5915
0x93c1b6 execute
	../../gcc-trunk/gcc/cfgexpand.c:6538
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.


> (with -march=armv5t: gcc.dg/pr83930.c (internal compiler error))
> 

possibly fixed by latest patch.

> on arm-linux-gnueabihf, in addition to align4/align_rec4:
> --with-cpu cortex-a9
> --with-fpu neon-fp16
>     gcc.c-torture/execute/pr37573.c   -O3 -fomit-frame-pointer
> -funroll-loops -fpeel-loops -ftracer -finline-functions  (internal
> compiler error)
>     gcc.c-torture/execute/pr37573.c   -O3 -g  (internal compiler error)
>     gcc.dg/vect/fast-math-pr35982.c (internal compiler error)
>     gcc.dg/vect/pr55857-1.c (internal compiler error)
>     gcc.dg/vect/pr55857-1.c -flto -ffat-lto-objects (internal compiler error)
>     gcc.dg/vect/pr55857-2.c (internal compiler error)
>     gcc.dg/vect/pr55857-2.c -flto -ffat-lto-objects (internal compiler error)
>     gcc.dg/vect/pr57558-2.c (internal compiler error)
>     gcc.dg/vect/pr57558-2.c -flto -ffat-lto-objects (internal compiler error)
> 
> and even more with other configs
> (http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/274986/report-build-info.html
> may help)
> 
> Christophe
> 
>>
>> $BUILD/arm-none-linux-gnueabihf/libstdc++-v3/include/ext/concurrence.h:
>> In function '(static initializers for
>> $SRC/libstdc++-v3/libsupc++/eh_alloc.cc)':
>> $BUILD/arm-none-linux-gnueabihf/libstdc++-v3/include/ext/concurrence.h:129:5:
>> internal compiler error: in gen_movv8qi, at config/arm/vec-common.md:29
>>    129 |     {
>>        |     ^
>> 0x14155cb gen_movv8qi(rtx_def*, rtx_def*)
>>          $SRC/gcc/config/arm/vec-common.md:29
>> 0x96bb89 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
>>          $SRC/gcc/recog.h:318
>> 0x94bc95 emit_move_insn_1(rtx_def*, rtx_def*)
>>          $SRC/gcc/expr.c:3694
>> 0x94c05b emit_move_insn(rtx_def*, rtx_def*)
>>          $SRC/gcc/expr.c:3790
>> 0x10d5ee5 arm_block_set_aligned_vect
>>          $SRC/gcc/config/arm/arm.c:30204
>> 0x10d6b37 arm_block_set_vect
>>          $SRC/gcc/config/arm/arm.c:30428
>> 0x10d6caf arm_gen_setmem(rtx_def**)
>>          $SRC/gcc/config/arm/arm.c:30458
>> 0x140d7ed gen_setmemsi(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
>>          $SRC/gcc/config/arm/arm.md:6687
>> 0xbf0e87 insn_gen_fn::operator()(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
>> const
>>          $SRC/gcc/recog.h:320
>> 0xbf0999 maybe_gen_insn(insn_code, unsigned int, expand_operand*)
>>          $SRC/gcc/optabs.c:7409
>> 0xbf0b87 maybe_expand_insn(insn_code, unsigned int, expand_operand*)
>>          $SRC/gcc/optabs.c:7440
>> 0x94a709 set_storage_via_setmem(rtx_def*, rtx_def*, rtx_def*, unsigned
>> int, unsigned int, long long, unsigned long long, unsigned long long,
>> unsigned long long)
>>          $SRC/gcc/expr.c:3168
>> 0x94a059 clear_storage_hints(rtx_def*, rtx_def*, block_op_methods,
>> unsigned int, long long, unsigned long long, unsigned long long,
>> unsigned long long)
>>          $SRC/gcc/expr.c:3037
>> 0x94a137 clear_storage(rtx_def*, rtx_def*, block_op_methods)
>>          $SRC/gcc/expr.c:3058
>> 0x9537c5 store_constructor
>>          $SRC/gcc/expr.c:6333
>> 0x957227 store_field
>>          $SRC/gcc/expr.c:7145
>> 0x94fde1 expand_assignment(tree_node*, tree_node*, bool)
>>          $SRC/gcc/expr.c:5301
>> 0x815e25 expand_gimple_stmt_1
>>          $SRC/gcc/cfgexpand.c:3777
>> 0x81611d expand_gimple_stmt
>>          $SRC/gcc/cfgexpand.c:3875
>> 0x81cd61 expand_gimple_basic_block
>>          $SRC/gcc/cfgexpand.c:5915
>>
>> Looks to me like arm_gen_setmem needs similar fixes to gen_cpymem_ldrd_strd?
>>

Yes, indeed, see attached patch.

This seems to fix the bootstrap, but at least one other error remains,
however I think those do hopefully not break the boot-strap and can be
fixed with follow-up patches.

Christophe can you please track the remaining regressions, that would be
very helpful.

Attached is an updated patch version which should un-break the bootstrap issues.
Is it OK for trunk?



Thanks
Bernd.
Kyrill Tkachov Aug. 30, 2019, 9:30 a.m. UTC | #13 
Hi Bernd,

On 8/29/19 10:26 PM, Bernd Edlinger wrote:
> On 8/29/19 11:08 AM, Christophe Lyon wrote:
>> On Thu, 29 Aug 2019 at 10:58, Kyrill Tkachov
>> <kyrylo.tkachov@foss.arm.com> wrote:
>>> Hi Bernd,
>>>
>>> On 8/28/19 10:36 PM, Bernd Edlinger wrote:
>>>> On 8/28/19 2:07 PM, Christophe Lyon wrote:
>>>>> Hi,
>>>>>
>>>>> This patch causes an ICE when building libgcc's unwind-arm.o
>>>>> when configuring GCC:
>>>>> --target  arm-none-linux-gnueabihf --with-mode thumb --with-cpu
>>>>> cortex-a15 --with-fpu neon-vfpv4:
>>>>>
>>>>> The build works for the same target, but --with-mode arm --with-cpu
>>>>> cortex a9 --with-fpu vfp
>>>>>
>>>>> In file included from
>>>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/config/arm/unwind-arm.c:144:
>>>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:
>>>>> In function 'get_eit_entry':
>>>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:245:29:
>>>>> warning: cast discards 'const' qualifier from pointer target type
>>>>> [-Wcast-qual]
>>>>>     245 |       ucbp->pr_cache.ehtp = (_Unwind_EHT_Header *)&eitp->content;
>>>>>         |                             ^
>>>>> during RTL pass: expand
>>>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:
>>>>> In function 'unwind_phase2_forced':
>>>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:319:18:
>>>>> internal compiler error: in gen_movdi, at config/arm/arm.md:5235
>>>>>     319 |   saved_vrs.core = entry_vrs->core;
>>>>>         |   ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
>>>>> 0x126530f gen_movdi(rtx_def*, rtx_def*)
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.md:5235
>>>>> 0x896d92 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/recog.h:318
>>>>> 0x896d92 emit_move_insn_1(rtx_def*, rtx_def*)
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:3694
>>>>> 0x897083 emit_move_insn(rtx_def*, rtx_def*)
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:3790
>>>>> 0xfc25d6 gen_cpymem_ldrd_strd(rtx_def**)
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.c:14582
>>>>> 0x126a1f1 gen_cpymemqi(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.md:6688
>>>>> 0xb0bc08 maybe_expand_insn(insn_code, unsigned int, expand_operand*)
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/optabs.c:7440
>>>>> 0x89ba1e emit_block_move_via_cpymem
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1808
>>>>> 0x89ba1e emit_block_move_hints(rtx_def*, rtx_def*, rtx_def*,
>>>>> block_op_methods, unsigned int, long, unsigned long, unsigned long,
>>>>> unsigned long, bool, bool*)
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1627
>>>>> 0x89c383 emit_block_move(rtx_def*, rtx_def*, rtx_def*, block_op_methods)
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1667
>>>>> 0x89fb4e store_expr(tree_node*, rtx_def*, int, bool, bool)
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:5845
>>>>> 0x88c1f9 store_field
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:7149
>>>>> 0x8a0c22 expand_assignment(tree_node*, tree_node*, bool)
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:5304
>>>>> 0x761964 expand_gimple_stmt_1
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:3779
>>>>> 0x761964 expand_gimple_stmt
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:3875
>>>>> 0x768583 expand_gimple_basic_block
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:5915
>>>>> 0x76abc6 execute
>>>>>           /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:6538
>>>>>
>>>>> Christophe
>>>>>
>>>> Okay, sorry for the breakage.
>>>>
>>>> What is happening in gen_cpymem_ldrd_strd is of course against the rules:
>>>>
>>>> It uses emit_move_insn on only 4-byte aligned DI-mode memory operands.
>>>>
>>>> I have a patch for this, which is able to fix the libgcc build on a cross, but have no
>>>> possibility to bootstrap the affected target.
>>>>
>>>> Could you please help?
>>> Well it's good that the sanitisation is catching the bugs!
>>>
> Yes, more than expected, though ;)
>
>>> Bootstrapping this patch I get another assert with the backtrace:
>> Thanks for the additional testing, Kyrill!
>>
>> FWIW, my original report was with a failure to just build GCC for
>> cortex-a15. I later got the reports of testing cross-toolchains, and
>> saw other problems on cortex-a9 for instance.
>> But I guess, you have noticed them with your bootstrap?
>> on arm-linux-gnueabi
>> gcc.target/arm/aapcs/align4.c (internal compiler error)
>> gcc.target/arm/aapcs/align_rec4.c (internal compiler error)
>>
> This appears to be yet unknown middle-end bug (not fixed by current patch)
>
> $ arm-linux-gnueabihf-gcc align4.c
> during RTL pass: expand
> In file included from align4.c:22:
> align4.c: In function 'testfunc':
> abitest.h:73:42: internal compiler error: in gen_movv2si, at config/arm/vec-common.md:30
>     73 | #define LAST_ARG(type,val,offset) { type __x = val; if (memcmp(&__x, stack+offset, sizeof(type)) != 0) abort(); }
>        |                                          ^~~
> abitest.h:74:30: note: in expansion of macro 'LAST_ARG'
>     74 | #define ARG(type,val,offset) LAST_ARG(type, val, offset)
>        |                              ^~~~~~~~
> align4.c:26:3: note: in expansion of macro 'ARG'
>     26 |   ARG (unalignedvec, a, R2)
>        |   ^~~
> 0x7bb33c gen_movv2si(rtx_def*, rtx_def*)
> 	../../gcc-trunk/gcc/config/arm/vec-common.md:30
> 0xa4a807 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
> 	../../gcc-trunk/gcc/recog.h:318
> 0xa4a807 emit_move_insn_1(rtx_def*, rtx_def*)
> 	../../gcc-trunk/gcc/expr.c:3694
> 0xa4ab94 emit_move_insn(rtx_def*, rtx_def*)
> 	../../gcc-trunk/gcc/expr.c:3790
> 0xa522bf store_expr(tree_node*, rtx_def*, int, bool, bool)
> 	../../gcc-trunk/gcc/expr.c:5855
> 0xa52bfd expand_assignment(tree_node*, tree_node*, bool)
> 	../../gcc-trunk/gcc/expr.c:5441
> 0xa52bfd expand_assignment(tree_node*, tree_node*, bool)
> 	../../gcc-trunk/gcc/expr.c:4982
> 0x934adf expand_gimple_stmt_1
> 	../../gcc-trunk/gcc/cfgexpand.c:3777
> 0x934adf expand_gimple_stmt
> 	../../gcc-trunk/gcc/cfgexpand.c:3875
> 0x93a451 expand_gimple_basic_block
> 	../../gcc-trunk/gcc/cfgexpand.c:5915
> 0x93c1b6 execute
> 	../../gcc-trunk/gcc/cfgexpand.c:6538
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See <https://gcc.gnu.org/bugs/> for instructions.
>
>
>> (with -march=armv5t: gcc.dg/pr83930.c (internal compiler error))
>>
> possibly fixed by latest patch.
>
>> on arm-linux-gnueabihf, in addition to align4/align_rec4:
>> --with-cpu cortex-a9
>> --with-fpu neon-fp16
>>      gcc.c-torture/execute/pr37573.c   -O3 -fomit-frame-pointer
>> -funroll-loops -fpeel-loops -ftracer -finline-functions  (internal
>> compiler error)
>>      gcc.c-torture/execute/pr37573.c   -O3 -g  (internal compiler error)
>>      gcc.dg/vect/fast-math-pr35982.c (internal compiler error)
>>      gcc.dg/vect/pr55857-1.c (internal compiler error)
>>      gcc.dg/vect/pr55857-1.c -flto -ffat-lto-objects (internal compiler error)
>>      gcc.dg/vect/pr55857-2.c (internal compiler error)
>>      gcc.dg/vect/pr55857-2.c -flto -ffat-lto-objects (internal compiler error)
>>      gcc.dg/vect/pr57558-2.c (internal compiler error)
>>      gcc.dg/vect/pr57558-2.c -flto -ffat-lto-objects (internal compiler error)
>>
>> and even more with other configs
>> (http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/274986/report-build-info.html
>> may help)
>>
>> Christophe
>>
>>> $BUILD/arm-none-linux-gnueabihf/libstdc++-v3/include/ext/concurrence.h:
>>> In function '(static initializers for
>>> $SRC/libstdc++-v3/libsupc++/eh_alloc.cc)':
>>> $BUILD/arm-none-linux-gnueabihf/libstdc++-v3/include/ext/concurrence.h:129:5:
>>> internal compiler error: in gen_movv8qi, at config/arm/vec-common.md:29
>>>     129 |     {
>>>         |     ^
>>> 0x14155cb gen_movv8qi(rtx_def*, rtx_def*)
>>>           $SRC/gcc/config/arm/vec-common.md:29
>>> 0x96bb89 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
>>>           $SRC/gcc/recog.h:318
>>> 0x94bc95 emit_move_insn_1(rtx_def*, rtx_def*)
>>>           $SRC/gcc/expr.c:3694
>>> 0x94c05b emit_move_insn(rtx_def*, rtx_def*)
>>>           $SRC/gcc/expr.c:3790
>>> 0x10d5ee5 arm_block_set_aligned_vect
>>>           $SRC/gcc/config/arm/arm.c:30204
>>> 0x10d6b37 arm_block_set_vect
>>>           $SRC/gcc/config/arm/arm.c:30428
>>> 0x10d6caf arm_gen_setmem(rtx_def**)
>>>           $SRC/gcc/config/arm/arm.c:30458
>>> 0x140d7ed gen_setmemsi(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
>>>           $SRC/gcc/config/arm/arm.md:6687
>>> 0xbf0e87 insn_gen_fn::operator()(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
>>> const
>>>           $SRC/gcc/recog.h:320
>>> 0xbf0999 maybe_gen_insn(insn_code, unsigned int, expand_operand*)
>>>           $SRC/gcc/optabs.c:7409
>>> 0xbf0b87 maybe_expand_insn(insn_code, unsigned int, expand_operand*)
>>>           $SRC/gcc/optabs.c:7440
>>> 0x94a709 set_storage_via_setmem(rtx_def*, rtx_def*, rtx_def*, unsigned
>>> int, unsigned int, long long, unsigned long long, unsigned long long,
>>> unsigned long long)
>>>           $SRC/gcc/expr.c:3168
>>> 0x94a059 clear_storage_hints(rtx_def*, rtx_def*, block_op_methods,
>>> unsigned int, long long, unsigned long long, unsigned long long,
>>> unsigned long long)
>>>           $SRC/gcc/expr.c:3037
>>> 0x94a137 clear_storage(rtx_def*, rtx_def*, block_op_methods)
>>>           $SRC/gcc/expr.c:3058
>>> 0x9537c5 store_constructor
>>>           $SRC/gcc/expr.c:6333
>>> 0x957227 store_field
>>>           $SRC/gcc/expr.c:7145
>>> 0x94fde1 expand_assignment(tree_node*, tree_node*, bool)
>>>           $SRC/gcc/expr.c:5301
>>> 0x815e25 expand_gimple_stmt_1
>>>           $SRC/gcc/cfgexpand.c:3777
>>> 0x81611d expand_gimple_stmt
>>>           $SRC/gcc/cfgexpand.c:3875
>>> 0x81cd61 expand_gimple_basic_block
>>>           $SRC/gcc/cfgexpand.c:5915
>>>
>>> Looks to me like arm_gen_setmem needs similar fixes to gen_cpymem_ldrd_strd?
>>>
> Yes, indeed, see attached patch.
>
> This seems to fix the bootstrap, but at least one other error remains,
> however I think those do hopefully not break the boot-strap and can be
> fixed with follow-up patches.
>
> Christophe can you please track the remaining regressions, that would be
> very helpful.
>
> Attached is an updated patch version which should un-break the bootstrap issues.
> Is it OK for trunk?
>
Yes, that fixes the bootstrap and testing looks ok, modulo the 
regressions Christophe listed.

Ok with one change...

+(define_insn "unaligned_storev8qi"
+  [(set (match_operand:V8QI 0 "memory_operand" "=Un")
+	(unspec:V8QI [(match_operand:V8QI 1 "s_register_operand" "w")]
+		     UNSPEC_UNALIGNED_STORE))]
+  "TARGET_NEON"
+  "*
+  return output_move_neon (operands);
+  "
+  [(set_attr "length" "4")
+   (set_attr "type" "neon_store1_1reg")])

No need to specify the "length" here as it's 4 by default.

Thanks,

Kyrill

>
> Thanks
> Bernd.
Christophe Lyon Aug. 30, 2019, 3:05 p.m. UTC | #14 
On Thu, 29 Aug 2019 at 23:26, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
>
> On 8/29/19 11:08 AM, Christophe Lyon wrote:
> > On Thu, 29 Aug 2019 at 10:58, Kyrill Tkachov
> > <kyrylo.tkachov@foss.arm.com> wrote:
> >>
> >> Hi Bernd,
> >>
> >> On 8/28/19 10:36 PM, Bernd Edlinger wrote:
> >>> On 8/28/19 2:07 PM, Christophe Lyon wrote:
> >>>> Hi,
> >>>>
> >>>> This patch causes an ICE when building libgcc's unwind-arm.o
> >>>> when configuring GCC:
> >>>> --target  arm-none-linux-gnueabihf --with-mode thumb --with-cpu
> >>>> cortex-a15 --with-fpu neon-vfpv4:
> >>>>
> >>>> The build works for the same target, but --with-mode arm --with-cpu
> >>>> cortex a9 --with-fpu vfp
> >>>>
> >>>> In file included from
> >>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/config/arm/unwind-arm.c:144:
> >>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:
> >>>> In function 'get_eit_entry':
> >>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:245:29:
> >>>> warning: cast discards 'const' qualifier from pointer target type
> >>>> [-Wcast-qual]
> >>>>    245 |       ucbp->pr_cache.ehtp = (_Unwind_EHT_Header *)&eitp->content;
> >>>>        |                             ^
> >>>> during RTL pass: expand
> >>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:
> >>>> In function 'unwind_phase2_forced':
> >>>> /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-arm-common.inc:319:18:
> >>>> internal compiler error: in gen_movdi, at config/arm/arm.md:5235
> >>>>    319 |   saved_vrs.core = entry_vrs->core;
> >>>>        |   ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
> >>>> 0x126530f gen_movdi(rtx_def*, rtx_def*)
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.md:5235
> >>>> 0x896d92 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/recog.h:318
> >>>> 0x896d92 emit_move_insn_1(rtx_def*, rtx_def*)
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:3694
> >>>> 0x897083 emit_move_insn(rtx_def*, rtx_def*)
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:3790
> >>>> 0xfc25d6 gen_cpymem_ldrd_strd(rtx_def**)
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.c:14582
> >>>> 0x126a1f1 gen_cpymemqi(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.md:6688
> >>>> 0xb0bc08 maybe_expand_insn(insn_code, unsigned int, expand_operand*)
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/optabs.c:7440
> >>>> 0x89ba1e emit_block_move_via_cpymem
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1808
> >>>> 0x89ba1e emit_block_move_hints(rtx_def*, rtx_def*, rtx_def*,
> >>>> block_op_methods, unsigned int, long, unsigned long, unsigned long,
> >>>> unsigned long, bool, bool*)
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1627
> >>>> 0x89c383 emit_block_move(rtx_def*, rtx_def*, rtx_def*, block_op_methods)
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:1667
> >>>> 0x89fb4e store_expr(tree_node*, rtx_def*, int, bool, bool)
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:5845
> >>>> 0x88c1f9 store_field
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:7149
> >>>> 0x8a0c22 expand_assignment(tree_node*, tree_node*, bool)
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/expr.c:5304
> >>>> 0x761964 expand_gimple_stmt_1
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:3779
> >>>> 0x761964 expand_gimple_stmt
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:3875
> >>>> 0x768583 expand_gimple_basic_block
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:5915
> >>>> 0x76abc6 execute
> >>>>          /tmp/6852788_4.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:6538
> >>>>
> >>>> Christophe
> >>>>
> >>> Okay, sorry for the breakage.
> >>>
> >>> What is happening in gen_cpymem_ldrd_strd is of course against the rules:
> >>>
> >>> It uses emit_move_insn on only 4-byte aligned DI-mode memory operands.
> >>>
> >>> I have a patch for this, which is able to fix the libgcc build on a cross, but have no
> >>> possibility to bootstrap the affected target.
> >>>
> >>> Could you please help?
> >>
> >> Well it's good that the sanitisation is catching the bugs!
> >>
>
> Yes, more than expected, though ;)
>
> >> Bootstrapping this patch I get another assert with the backtrace:
> >
> > Thanks for the additional testing, Kyrill!
> >
> > FWIW, my original report was with a failure to just build GCC for
> > cortex-a15. I later got the reports of testing cross-toolchains, and
> > saw other problems on cortex-a9 for instance.
> > But I guess, you have noticed them with your bootstrap?
> > on arm-linux-gnueabi
> > gcc.target/arm/aapcs/align4.c (internal compiler error)
> > gcc.target/arm/aapcs/align_rec4.c (internal compiler error)
> >
>
> This appears to be yet unknown middle-end bug (not fixed by current patch)
>
> $ arm-linux-gnueabihf-gcc align4.c
> during RTL pass: expand
> In file included from align4.c:22:
> align4.c: In function 'testfunc':
> abitest.h:73:42: internal compiler error: in gen_movv2si, at config/arm/vec-common.md:30
>    73 | #define LAST_ARG(type,val,offset) { type __x = val; if (memcmp(&__x, stack+offset, sizeof(type)) != 0) abort(); }
>       |                                          ^~~
> abitest.h:74:30: note: in expansion of macro 'LAST_ARG'
>    74 | #define ARG(type,val,offset) LAST_ARG(type, val, offset)
>       |                              ^~~~~~~~
> align4.c:26:3: note: in expansion of macro 'ARG'
>    26 |   ARG (unalignedvec, a, R2)
>       |   ^~~
> 0x7bb33c gen_movv2si(rtx_def*, rtx_def*)
>         ../../gcc-trunk/gcc/config/arm/vec-common.md:30
> 0xa4a807 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
>         ../../gcc-trunk/gcc/recog.h:318
> 0xa4a807 emit_move_insn_1(rtx_def*, rtx_def*)
>         ../../gcc-trunk/gcc/expr.c:3694
> 0xa4ab94 emit_move_insn(rtx_def*, rtx_def*)
>         ../../gcc-trunk/gcc/expr.c:3790
> 0xa522bf store_expr(tree_node*, rtx_def*, int, bool, bool)
>         ../../gcc-trunk/gcc/expr.c:5855
> 0xa52bfd expand_assignment(tree_node*, tree_node*, bool)
>         ../../gcc-trunk/gcc/expr.c:5441
> 0xa52bfd expand_assignment(tree_node*, tree_node*, bool)
>         ../../gcc-trunk/gcc/expr.c:4982
> 0x934adf expand_gimple_stmt_1
>         ../../gcc-trunk/gcc/cfgexpand.c:3777
> 0x934adf expand_gimple_stmt
>         ../../gcc-trunk/gcc/cfgexpand.c:3875
> 0x93a451 expand_gimple_basic_block
>         ../../gcc-trunk/gcc/cfgexpand.c:5915
> 0x93c1b6 execute
>         ../../gcc-trunk/gcc/cfgexpand.c:6538
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See <https://gcc.gnu.org/bugs/> for instructions.
>
>
> > (with -march=armv5t: gcc.dg/pr83930.c (internal compiler error))
> >
>
> possibly fixed by latest patch.
>
> > on arm-linux-gnueabihf, in addition to align4/align_rec4:
> > --with-cpu cortex-a9
> > --with-fpu neon-fp16
> >     gcc.c-torture/execute/pr37573.c   -O3 -fomit-frame-pointer
> > -funroll-loops -fpeel-loops -ftracer -finline-functions  (internal
> > compiler error)
> >     gcc.c-torture/execute/pr37573.c   -O3 -g  (internal compiler error)
> >     gcc.dg/vect/fast-math-pr35982.c (internal compiler error)
> >     gcc.dg/vect/pr55857-1.c (internal compiler error)
> >     gcc.dg/vect/pr55857-1.c -flto -ffat-lto-objects (internal compiler error)
> >     gcc.dg/vect/pr55857-2.c (internal compiler error)
> >     gcc.dg/vect/pr55857-2.c -flto -ffat-lto-objects (internal compiler error)
> >     gcc.dg/vect/pr57558-2.c (internal compiler error)
> >     gcc.dg/vect/pr57558-2.c -flto -ffat-lto-objects (internal compiler error)
> >
> > and even more with other configs
> > (http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/274986/report-build-info.html
> > may help)
> >
> > Christophe
> >
> >>
> >> $BUILD/arm-none-linux-gnueabihf/libstdc++-v3/include/ext/concurrence.h:
> >> In function '(static initializers for
> >> $SRC/libstdc++-v3/libsupc++/eh_alloc.cc)':
> >> $BUILD/arm-none-linux-gnueabihf/libstdc++-v3/include/ext/concurrence.h:129:5:
> >> internal compiler error: in gen_movv8qi, at config/arm/vec-common.md:29
> >>    129 |     {
> >>        |     ^
> >> 0x14155cb gen_movv8qi(rtx_def*, rtx_def*)
> >>          $SRC/gcc/config/arm/vec-common.md:29
> >> 0x96bb89 insn_gen_fn::operator()(rtx_def*, rtx_def*) const
> >>          $SRC/gcc/recog.h:318
> >> 0x94bc95 emit_move_insn_1(rtx_def*, rtx_def*)
> >>          $SRC/gcc/expr.c:3694
> >> 0x94c05b emit_move_insn(rtx_def*, rtx_def*)
> >>          $SRC/gcc/expr.c:3790
> >> 0x10d5ee5 arm_block_set_aligned_vect
> >>          $SRC/gcc/config/arm/arm.c:30204
> >> 0x10d6b37 arm_block_set_vect
> >>          $SRC/gcc/config/arm/arm.c:30428
> >> 0x10d6caf arm_gen_setmem(rtx_def**)
> >>          $SRC/gcc/config/arm/arm.c:30458
> >> 0x140d7ed gen_setmemsi(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
> >>          $SRC/gcc/config/arm/arm.md:6687
> >> 0xbf0e87 insn_gen_fn::operator()(rtx_def*, rtx_def*, rtx_def*, rtx_def*)
> >> const
> >>          $SRC/gcc/recog.h:320
> >> 0xbf0999 maybe_gen_insn(insn_code, unsigned int, expand_operand*)
> >>          $SRC/gcc/optabs.c:7409
> >> 0xbf0b87 maybe_expand_insn(insn_code, unsigned int, expand_operand*)
> >>          $SRC/gcc/optabs.c:7440
> >> 0x94a709 set_storage_via_setmem(rtx_def*, rtx_def*, rtx_def*, unsigned
> >> int, unsigned int, long long, unsigned long long, unsigned long long,
> >> unsigned long long)
> >>          $SRC/gcc/expr.c:3168
> >> 0x94a059 clear_storage_hints(rtx_def*, rtx_def*, block_op_methods,
> >> unsigned int, long long, unsigned long long, unsigned long long,
> >> unsigned long long)
> >>          $SRC/gcc/expr.c:3037
> >> 0x94a137 clear_storage(rtx_def*, rtx_def*, block_op_methods)
> >>          $SRC/gcc/expr.c:3058
> >> 0x9537c5 store_constructor
> >>          $SRC/gcc/expr.c:6333
> >> 0x957227 store_field
> >>          $SRC/gcc/expr.c:7145
> >> 0x94fde1 expand_assignment(tree_node*, tree_node*, bool)
> >>          $SRC/gcc/expr.c:5301
> >> 0x815e25 expand_gimple_stmt_1
> >>          $SRC/gcc/cfgexpand.c:3777
> >> 0x81611d expand_gimple_stmt
> >>          $SRC/gcc/cfgexpand.c:3875
> >> 0x81cd61 expand_gimple_basic_block
> >>          $SRC/gcc/cfgexpand.c:5915
> >>
> >> Looks to me like arm_gen_setmem needs similar fixes to gen_cpymem_ldrd_strd?
> >>
>
> Yes, indeed, see attached patch.
>
> This seems to fix the bootstrap, but at least one other error remains,
> however I think those do hopefully not break the boot-strap and can be
> fixed with follow-up patches.
>
> Christophe can you please track the remaining regressions, that would be
> very helpful.
>
> Attached is an updated patch version which should un-break the bootstrap issues.
> Is it OK for trunk?
>

I've run validations comparing r274985 (rev before your 1st patch)
with r274986 and the patch from this thread.
I think you've committed it while the validations were running.

I've filed several PR for the different ICEs and regressions I've noticed:
91612 91613 61614 91615

HTH

Thanks,

Christophe


>
> Thanks
> Bernd.
diff mbox series

Patch

2019-08-15  Bernd Edlinger  <bernd.edlinger@hotmail.de>
	    Richard Biener  <rguenther@suse.de>

	* expr.c (expand_assignment): Handle misaligned DECLs.
	(expand_expr_real_1): Handle FUNCTION_DECL as unaligned.
	* function.c (assign_parm_adjust_stack_rtl): Check movmisalign optab
	too.
	(assign_parm_setup_stack): Allocate properly aligned stack slots.
	* varasm.c (build_constant_desc): Align constants of misaligned types.
	* config/arm/arm.md (movdi, movsi, movhi, movhf, movsf, movdf): Check
	strict alignment restrictions on memory addresses.
	* config/arm/neon.md (movti, mov<VSTRUCT>, mov<VH>): Likewise.
	* config/arm/vec-common.md (mov<VALL>): Likewise.

Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md	(Revision 274531)
+++ gcc/config/arm/arm.md	(Arbeitskopie)
@@ -5838,6 +5838,12 @@ 
 	(match_operand:DI 1 "general_operand"))]
   "TARGET_EITHER"
   "
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (DImode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (DImode));
   if (can_create_pseudo_p ())
     {
       if (!REG_P (operands[0]))
@@ -6014,6 +6020,12 @@ 
   {
   rtx base, offset, tmp;
 
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (SImode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (SImode));
   if (TARGET_32BIT || TARGET_HAVE_MOVT)
     {
       /* Everything except mem = const or mem = mem can be done easily.  */
@@ -6503,6 +6515,12 @@ 
 	(match_operand:HI 1 "general_operand"))]
   "TARGET_EITHER"
   "
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (HImode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (HImode));
   if (TARGET_ARM)
     {
       if (can_create_pseudo_p ())
@@ -6912,6 +6930,12 @@ 
 	(match_operand:HF 1 "general_operand"))]
   "TARGET_EITHER"
   "
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (HFmode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (HFmode));
   if (TARGET_32BIT)
     {
       if (MEM_P (operands[0]))
@@ -6976,6 +7000,12 @@ 
 	(match_operand:SF 1 "general_operand"))]
   "TARGET_EITHER"
   "
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (SFmode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (SFmode));
   if (TARGET_32BIT)
     {
       if (MEM_P (operands[0]))
@@ -7071,6 +7101,12 @@ 
 	(match_operand:DF 1 "general_operand"))]
   "TARGET_EITHER"
   "
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (DFmode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (DFmode));
   if (TARGET_32BIT)
     {
       if (MEM_P (operands[0]))
Index: gcc/config/arm/neon.md
===================================================================
--- gcc/config/arm/neon.md	(Revision 274531)
+++ gcc/config/arm/neon.md	(Arbeitskopie)
@@ -127,6 +127,12 @@ 
 	(match_operand:TI 1 "general_operand"))]
   "TARGET_NEON"
 {
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (TImode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (TImode));
   if (can_create_pseudo_p ())
     {
       if (!REG_P (operands[0]))
@@ -139,6 +145,12 @@ 
 	(match_operand:VSTRUCT 1 "general_operand"))]
   "TARGET_NEON"
 {
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (<MODE>mode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (<MODE>mode));
   if (can_create_pseudo_p ())
     {
       if (!REG_P (operands[0]))
@@ -151,6 +163,12 @@ 
 	(match_operand:VH 1 "s_register_operand"))]
   "TARGET_NEON"
 {
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (<MODE>mode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (<MODE>mode));
   if (can_create_pseudo_p ())
     {
       if (!REG_P (operands[0]))
Index: gcc/config/arm/vec-common.md
===================================================================
--- gcc/config/arm/vec-common.md	(Revision 274531)
+++ gcc/config/arm/vec-common.md	(Arbeitskopie)
@@ -26,6 +26,12 @@ 
   "TARGET_NEON
    || (TARGET_REALLY_IWMMXT && VALID_IWMMXT_REG_MODE (<MODE>mode))"
 {
+  gcc_checking_assert (!MEM_P (operands[0])
+		       || MEM_ALIGN (operands[0])
+			  >= GET_MODE_ALIGNMENT (<MODE>mode));
+  gcc_checking_assert (!MEM_P (operands[1])
+		       || MEM_ALIGN (operands[1])
+			  >= GET_MODE_ALIGNMENT (<MODE>mode));
   if (can_create_pseudo_p ())
     {
       if (!REG_P (operands[0]))
Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(Revision 274531)
+++ gcc/expr.c	(Arbeitskopie)
@@ -5002,9 +5002,10 @@  expand_assignment (tree to, tree from, bool nontem
   /* Handle misaligned stores.  */
   mode = TYPE_MODE (TREE_TYPE (to));
   if ((TREE_CODE (to) == MEM_REF
-       || TREE_CODE (to) == TARGET_MEM_REF)
+       || TREE_CODE (to) == TARGET_MEM_REF
+       || DECL_P (to))
       && mode != BLKmode
-      && !mem_ref_refers_to_non_mem_p (to)
+      && (DECL_P (to) || !mem_ref_refers_to_non_mem_p (to))
       && ((align = get_object_alignment (to))
 	  < GET_MODE_ALIGNMENT (mode))
       && (((icode = optab_handler (movmisalign_optab, mode))
@@ -10796,6 +10797,14 @@  expand_expr_real_1 (tree exp, rtx target, machine_
 	    MEM_VOLATILE_P (op0) = 1;
 	  }
 
+	if (MEM_P (op0) && TREE_CODE (tem) == FUNCTION_DECL)
+	  {
+	    if (op0 == orig_op0)
+	      op0 = copy_rtx (op0);
+
+	    set_mem_align (op0, BITS_PER_UNIT);
+	  }
+
 	/* In cases where an aligned union has an unaligned object
 	   as a field, we might be extracting a BLKmode value from
 	   an integer-mode (e.g., SImode) object.  Handle this case
Index: gcc/function.c
===================================================================
--- gcc/function.c	(Revision 274531)
+++ gcc/function.c	(Arbeitskopie)
@@ -2812,8 +2827,10 @@  assign_parm_adjust_stack_rtl (struct assign_parm_d
      stack slot, if we need one.  */
   if (stack_parm
       && ((GET_MODE_ALIGNMENT (data->nominal_mode) > MEM_ALIGN (stack_parm)
-	   && targetm.slow_unaligned_access (data->nominal_mode,
-					     MEM_ALIGN (stack_parm)))
+	   && ((optab_handler (movmisalign_optab, data->nominal_mode)
+		!= CODE_FOR_nothing)
+	       || targetm.slow_unaligned_access (data->nominal_mode,
+						 MEM_ALIGN (stack_parm))))
 	  || (data->nominal_type
 	      && TYPE_ALIGN (data->nominal_type) > MEM_ALIGN (stack_parm)
 	      && MEM_ALIGN (stack_parm) < PREFERRED_STACK_BOUNDARY)))
@@ -3466,11 +3483,20 @@  assign_parm_setup_stack (struct assign_parm_data_a
 	  int align = STACK_SLOT_ALIGNMENT (data->passed_type,
 					    GET_MODE (data->entry_parm),
 					    TYPE_ALIGN (data->passed_type));
+	  if (align < (int)GET_MODE_ALIGNMENT (GET_MODE (data->entry_parm))
+	      && ((optab_handler (movmisalign_optab,
+				  GET_MODE (data->entry_parm))
+		   != CODE_FOR_nothing)
+		  || targetm.slow_unaligned_access (GET_MODE (data->entry_parm),
+						    align)))
+	    align = GET_MODE_ALIGNMENT (GET_MODE (data->entry_parm));
 	  data->stack_parm
 	    = assign_stack_local (GET_MODE (data->entry_parm),
 				  GET_MODE_SIZE (GET_MODE (data->entry_parm)),
 				  align);
+	  align = MEM_ALIGN (data->stack_parm);
 	  set_mem_attributes (data->stack_parm, parm, 1);
+	  set_mem_align (data->stack_parm, align);
 	}
 
       dest = validize_mem (copy_rtx (data->stack_parm));
Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c	(Revision 274531)
+++ gcc/varasm.c	(Arbeitskopie)
@@ -47,6 +47,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "stmt.h"
 #include "expr.h"
 #include "expmed.h"
+#include "optabs.h"
 #include "output.h"
 #include "langhooks.h"
 #include "debug.h"
@@ -3386,7 +3387,15 @@  build_constant_desc (tree exp)
   if (TREE_CODE (exp) == STRING_CST)
     SET_DECL_ALIGN (decl, targetm.constant_alignment (exp, DECL_ALIGN (decl)));
   else
-    align_variable (decl, 0);
+    {
+      align_variable (decl, 0);
+      if (DECL_ALIGN (decl) < GET_MODE_ALIGNMENT (DECL_MODE (decl))
+	  && ((optab_handler (movmisalign_optab, DECL_MODE (decl))
+	       != CODE_FOR_nothing)
+	      || targetm.slow_unaligned_access (DECL_MODE (decl),
+						DECL_ALIGN (decl))))
+	SET_DECL_ALIGN (decl, GET_MODE_ALIGNMENT (DECL_MODE (decl)));
+    }
 
   /* Now construct the SYMBOL_REF and the MEM.  */
   if (use_object_blocks_p ())