[committed] Remove compromised sh test

[committed] Remove compromised sh test

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]

Surya's recent patch to IRA improves the code for sh/pr54602-1.c 
slightly.  Specifically it's able to eliminate a save/restore in the 
prologue/epilogue and a bit of register shuffling.

As a result there literally aren't any insns that can be used to fill 
the delay slot of the return, so a nop gets emitted and the test fails.

Given there literally aren't any insns to move into the delay slot, the 
best course of action is to just drop the test.

Pushed to the trunk.

Jeff

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 1375 bytes --]

commit 47b68cda2c4afe32e84c5f18da0196c39e5e0edf
Author: Jeff Law <jlaw@ventanamicro.com>
Date:   Wed Jun 26 07:20:29 2024 -0600

    [committed] Remove compromised sh test
    
    Surya's recent patch to IRA improves the code for sh/pr54602-1.c slightly.
    Specifically it's able to eliminate a save/restore in the prologue/epilogue and
    a bit of register shuffling.
    
    As a result there literally aren't any insns that can be used to fill the delay
    slot of the return, so a nop gets emitted and the test fails.
    
    Given there literally aren't any insns to move into the delay slot, the best
    course of action is to just drop the test.
    
    gcc/testsuite
            * gcc.target/sh/pr54602-1.c: Delete test.

diff --git a/gcc/testsuite/gcc.target/sh/pr54602-1.c b/gcc/testsuite/gcc.target/sh/pr54602-1.c
deleted file mode 100644
index e7fb2a9a642..00000000000
--- a/gcc/testsuite/gcc.target/sh/pr54602-1.c
+++ /dev/null
@@ -1,14 +0,0 @@
-/* Verify that the delay slot is stuffed with register pop insns for normal
-   (i.e. not interrupt handler) function returns.  If everything goes as
-   expected we won't see any nop insns.  */
-/* { dg-do compile }  */
-/* { dg-options "-O1" } */
-/* { dg-final { scan-assembler-not "nop" } } */
-
-int test00 (int a, int b);
-
-int
-test01 (int a, int b, int c, int d)
-{
-  return test00 (a, b) + c;
-}

Re: [committed] Remove compromised sh test

[Oleg Endo]

On Wed, 2024-06-26 at 07:22 -0600, Jeff Law wrote:
> Surya's recent patch to IRA improves the code for sh/pr54602-1.c 
> slightly.  Specifically it's able to eliminate a save/restore in the 
> prologue/epilogue and a bit of register shuffling.
> 
> As a result there literally aren't any insns that can be used to fill 
> the delay slot of the return, so a nop gets emitted and the test fails.
> 
> Given there literally aren't any insns to move into the delay slot, the 
> best course of action is to just drop the test.
> 
> Pushed to the trunk.
> 
> Jeff

I can't reproduce what you are saying.
Which triplet and flags is your test setup using?

For this test case, GCC 13 with -m4 -ml -O1 -fno-pic:

_test01:
        mov.l   r8,@-r15
        sts.l   pr,@-r15
        mov.l   .L3,r0
        jsr     @r0
        mov     r6,r8
        add     r8,r0
        lds.l   @r15+,pr
        rts     
        mov.l   @r15+,r8
.L3:
        .long   _test00


current GCC master branch with -m4 -ml -O1 -fno-pic:

_test00:
	mov.l	r8,@-r15
	sts.l	pr,@-r15
	mov.l	.L3,r0
	jsr	@r0
	mov	r6,r8
	add	r8,r0
	lds.l	@r15+,pr
	rts
	mov.l	@r15+,r8
.L4:
	.align 2
.L3:
	.long	_test01


Best regards,
Oleg Endo

Re: [committed] Remove compromised sh test

[Jeff Law]

On 6/26/24 4:12 PM, Oleg Endo wrote:
> 
> 
> On Wed, 2024-06-26 at 07:22 -0600, Jeff Law wrote:
>> Surya's recent patch to IRA improves the code for sh/pr54602-1.c
>> slightly.  Specifically it's able to eliminate a save/restore in the
>> prologue/epilogue and a bit of register shuffling.
>>
>> As a result there literally aren't any insns that can be used to fill
>> the delay slot of the return, so a nop gets emitted and the test fails.
>>
>> Given there literally aren't any insns to move into the delay slot, the
>> best course of action is to just drop the test.
>>
>> Pushed to the trunk.
>>
>> Jeff
> 
> I can't reproduce what you are saying.
> Which triplet and flags is your test setup using?
> 
> For this test case, GCC 13 with -m4 -ml -O1 -fno-pic:
No -m flags at all.   As plain of a testrun as you can do.

jeff

Re: [committed] Remove compromised sh test

[Oleg Endo]

On Wed, 2024-06-26 at 16:39 -0600, Jeff Law wrote:
> 
> On 6/26/24 4:12 PM, Oleg Endo wrote:
> > 
> > 
> > On Wed, 2024-06-26 at 07:22 -0600, Jeff Law wrote:
> > > Surya's recent patch to IRA improves the code for sh/pr54602-1.c
> > > slightly.  Specifically it's able to eliminate a save/restore in the
> > > prologue/epilogue and a bit of register shuffling.
> > > 
> > > As a result there literally aren't any insns that can be used to fill
> > > the delay slot of the return, so a nop gets emitted and the test fails.
> > > 
> > > Given there literally aren't any insns to move into the delay slot, the
> > > best course of action is to just drop the test.
> > > 
> > > Pushed to the trunk.
> > > 
> > > Jeff
> > 
> > I can't reproduce what you are saying.
> > Which triplet and flags is your test setup using?
> > 
> > For this test case, GCC 13 with -m4 -ml -O1 -fno-pic:
> No -m flags at all.   As plain of a testrun as you can do.
> 

OK, then what's the default config of your test setup / triplet?
Can you please show the generated code that you get?  Because - like I said
- I can't reproduce it.

Best regards,
Oleg Endo

Re: [committed] Remove compromised sh test

[Jeff Law]

On 6/26/24 4:44 PM, Oleg Endo wrote:
> 
> 
> On Wed, 2024-06-26 at 16:39 -0600, Jeff Law wrote:
>>
>> On 6/26/24 4:12 PM, Oleg Endo wrote:
>>>
>>>
>>> On Wed, 2024-06-26 at 07:22 -0600, Jeff Law wrote:
>>>> Surya's recent patch to IRA improves the code for sh/pr54602-1.c
>>>> slightly.  Specifically it's able to eliminate a save/restore in the
>>>> prologue/epilogue and a bit of register shuffling.
>>>>
>>>> As a result there literally aren't any insns that can be used to fill
>>>> the delay slot of the return, so a nop gets emitted and the test fails.
>>>>
>>>> Given there literally aren't any insns to move into the delay slot, the
>>>> best course of action is to just drop the test.
>>>>
>>>> Pushed to the trunk.
>>>>
>>>> Jeff
>>>
>>> I can't reproduce what you are saying.
>>> Which triplet and flags is your test setup using?
>>>
>>> For this test case, GCC 13 with -m4 -ml -O1 -fno-pic:
>> No -m flags at all.   As plain of a testrun as you can do.
>>
> 
> OK, then what's the default config of your test setup / triplet?
> Can you please show the generated code that you get?  Because - like I said
> - I can't reproduce it.
test01:
         sts.l   pr,@-r15        ! 31    [c=4 l=2]  movsi_i/10
         add     #-4,r15 ! 32    [c=4 l=2]  *addsi3/0
         mov.l   .L3,r0  ! 26    [c=10 l=2]  movsi_i/0
         jsr     @r0     ! 12    [c=5 l=2]  call_valuei
         mov.l   r6,@r15 ! 4     [c=4 l=2]  movsi_i/8
         mov.l   @r15,r1 ! 29    [c=1 l=2]  movsi_i/5
         add     r1,r0   ! 30    [c=4 l=2]  *addsi3/0
         add     #4,r15  ! 36    [c=4 l=2]  *addsi3/0
         lds.l   @r15+,pr        ! 38    [c=1 l=2]  movsi_i/14
         rts
         nop             ! 40    [c=0 l=4]  *return_i


Note that there's a scheduling barrier in the RTL between insns 30 and 
36.  So instructions prior to insn 36 can't be used to fill the delay slot.

jeff

Re: [committed] Remove compromised sh test

[Oleg Endo]

On Wed, 2024-06-26 at 18:30 -0600, Jeff Law wrote:
> > > 
> > 
> > OK, then what's the default config of your test setup / triplet?
> > Can you please show the generated code that you get?  Because - like I said
> > - I can't reproduce it.
> test01:
>          sts.l   pr,@-r15        ! 31    [c=4 l=2]  movsi_i/10
>          add     #-4,r15 ! 32    [c=4 l=2]  *addsi3/0
>          mov.l   .L3,r0  ! 26    [c=10 l=2]  movsi_i/0
>          jsr     @r0     ! 12    [c=5 l=2]  call_valuei
>          mov.l   r6,@r15 ! 4     [c=4 l=2]  movsi_i/8
>          mov.l   @r15,r1 ! 29    [c=1 l=2]  movsi_i/5
>          add     r1,r0   ! 30    [c=4 l=2]  *addsi3/0
>          add     #4,r15  ! 36    [c=4 l=2]  *addsi3/0
>          lds.l   @r15+,pr        ! 38    [c=1 l=2]  movsi_i/14
>          rts
>          nop             ! 40    [c=0 l=4]  *return_i
> 
> 
> Note that there's a scheduling barrier in the RTL between insns 30 and 
> 36.  So instructions prior to insn 36 can't be used to fill the delay slot.
> 

Thanks.  Now I'm also seeing the same result.  Needed to specify -O2 to get
that.  -O1 was not enough it seems.

I don't know why you said that the code for this case improved -- it has
not?!

I think the test is still valid.  The reason for the failure might be
different from the original one (the scheduling barrier for whatever
reason), but the end result is the same -- the last delay slot is not
stuffed, although the 'add r1,r0' could go in there.

I'd like to revert the removal of this test case, as it catches a valid
issue.

Best regards,
Oleg Endo

Re: [committed] Remove compromised sh test

[Jeff Law]

On 6/26/24 7:27 PM, Oleg Endo wrote:
> On Wed, 2024-06-26 at 18:30 -0600, Jeff Law wrote:
>>>>
>>>
>>> OK, then what's the default config of your test setup / triplet?
>>> Can you please show the generated code that you get?  Because - like I said
>>> - I can't reproduce it.
>> test01:
>>           sts.l   pr,@-r15        ! 31    [c=4 l=2]  movsi_i/10
>>           add     #-4,r15 ! 32    [c=4 l=2]  *addsi3/0
>>           mov.l   .L3,r0  ! 26    [c=10 l=2]  movsi_i/0
>>           jsr     @r0     ! 12    [c=5 l=2]  call_valuei
>>           mov.l   r6,@r15 ! 4     [c=4 l=2]  movsi_i/8
>>           mov.l   @r15,r1 ! 29    [c=1 l=2]  movsi_i/5
>>           add     r1,r0   ! 30    [c=4 l=2]  *addsi3/0
>>           add     #4,r15  ! 36    [c=4 l=2]  *addsi3/0
>>           lds.l   @r15+,pr        ! 38    [c=1 l=2]  movsi_i/14
>>           rts
>>           nop             ! 40    [c=0 l=4]  *return_i
>>
>>
>> Note that there's a scheduling barrier in the RTL between insns 30 and
>> 36.  So instructions prior to insn 36 can't be used to fill the delay slot.
>>
> 
> Thanks.  Now I'm also seeing the same result.  Needed to specify -O2 to get
> that.  -O1 was not enough it seems.
> 
> I don't know why you said that the code for this case improved -- it has
> not?!
> 
> I think the test is still valid.  The reason for the failure might be
> different from the original one (the scheduling barrier for whatever
> reason), but the end result is the same -- the last delay slot is not
> stuffed, although the 'add r1,r0' could go in there.
> 
> I'd like to revert the removal of this test case, as it catches a valid
> issue.

Before the IRA patch there is an additional prologue/epilogue 
save/restore for a callee saved register.  That's what filled the delay 
slot before.

THe add r1,r0 can not move down to fill the delay slot.  There's 
scheduling barrier in the RTL.

Feel free to restore it, but you're just adding a bogus, failing, test 
to the testsuite.

jeff

[committed] Remove compromised sh test

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]

Surya's recent patch to IRA improves the code for sh/pr54602-1.c 
slightly.  Specifically it's able to eliminate a save/restore in the 
prologue/epilogue and a bit of register shuffling.

As a result there literally aren't any insns that can be used to fill 
the delay slot of the return, so a nop gets emitted and the test fails.

Given there literally aren't any insns to move into the delay slot, the 
best course of action is to just drop the test.

Pushed to the trunk.

Jeff

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 1375 bytes --]

commit 47b68cda2c4afe32e84c5f18da0196c39e5e0edf
Author: Jeff Law <jlaw@ventanamicro.com>
Date:   Wed Jun 26 07:20:29 2024 -0600

    [committed] Remove compromised sh test
    
    Surya's recent patch to IRA improves the code for sh/pr54602-1.c slightly.
    Specifically it's able to eliminate a save/restore in the prologue/epilogue and
    a bit of register shuffling.
    
    As a result there literally aren't any insns that can be used to fill the delay
    slot of the return, so a nop gets emitted and the test fails.
    
    Given there literally aren't any insns to move into the delay slot, the best
    course of action is to just drop the test.
    
    gcc/testsuite
            * gcc.target/sh/pr54602-1.c: Delete test.

diff --git a/gcc/testsuite/gcc.target/sh/pr54602-1.c b/gcc/testsuite/gcc.target/sh/pr54602-1.c
deleted file mode 100644
index e7fb2a9a642..00000000000
--- a/gcc/testsuite/gcc.target/sh/pr54602-1.c
+++ /dev/null
@@ -1,14 +0,0 @@
-/* Verify that the delay slot is stuffed with register pop insns for normal
-   (i.e. not interrupt handler) function returns.  If everything goes as
-   expected we won't see any nop insns.  */
-/* { dg-do compile }  */
-/* { dg-options "-O1" } */
-/* { dg-final { scan-assembler-not "nop" } } */
-
-int test00 (int a, int b);
-
-int
-test01 (int a, int b, int c, int d)
-{
-  return test00 (a, b) + c;
-}