[committed obvious][arm] Add test that was missing from old commit [PR91816]

[committed obvious][arm] Add test that was missing from old commit [PR91816]

[Stam Markianos-Wright]

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

Hi all,

A while back I submitted GCC10 commit:

  44f77a6dea2f312ee1743f3dde465c1b8453ee13

for PR91816.

Turns out I was an idiot and forgot to include the test in the actual 
git commit, even my entire patch had been approved.

Tested that the test still passes on a cross arm-none-eabi and also in a
Cortex A-15 bootstrap with no regressions.

Submitting this as Obvious to gcc-11 and backporting to gcc-10.

Thanks,
Stam Markianos-Wright

gcc/testsuite/ChangeLog:
	PR target/91816
	* gcc.target/arm/pr91816.c: New test.

[-- Attachment #2: PR91816-test-only.patch --]
[-- Type: text/x-patch, Size: 1563 bytes --]

diff --git a/gcc/testsuite/gcc.target/arm/pr91816.c b/gcc/testsuite/gcc.target/arm/pr91816.c
new file mode 100644
index 00000000000..75b938a6aad
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr91816.c
@@ -0,0 +1,63 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_thumb2_ok } */
+/* { dg-additional-options "-mthumb" }  */
+/* { dg-timeout-factor 4.0 } */
+
+int printf(const char *, ...);
+
+#define HW0	printf("Hello World!\n");
+#define HW1	HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0 HW0
+#define HW2	HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1 HW1
+#define HW3	HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2
+#define HW4	HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3
+#define HW5	HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4
+
+__attribute__((noinline,noclone)) void f1 (int a)
+{
+  if (a) { HW0 }
+}
+
+__attribute__((noinline,noclone)) void f2 (int a)
+{
+  if (a) { HW3 }
+}
+
+
+__attribute__((noinline,noclone)) void f3 (int a)
+{
+  if (a) { HW5 }
+}
+
+__attribute__((noinline,noclone)) void f4 (int a)
+{
+  if (a == 1) { HW0 }
+}
+
+__attribute__((noinline,noclone)) void f5 (int a)
+{
+  if (a == 1) { HW3 }
+}
+
+
+__attribute__((noinline,noclone)) void f6 (int a)
+{
+  if (a == 1) { HW5 }
+}
+
+
+int main(void)
+{
+	f1(0);
+	f2(0);
+	f3(0);
+	f4(0);
+	f5(0);
+	f6(0);
+	return 0;
+}
+
+
+/* { dg-final { scan-assembler-times "beq\\t.L\[0-9\]" 2 } } */
+/* { dg-final { scan-assembler-times "beq\\t.Lbcond\[0-9\]" 1 } } */
+/* { dg-final { scan-assembler-times "bne\\t.L\[0-9\]" 2 } } */
+/* { dg-final { scan-assembler-times "bne\\t.Lbcond\[0-9\]" 1 } } */

Re: [committed obvious][arm] Add test that was missing from old commit [PR91816]

[Christophe Lyon]

On Wed, 25 Nov 2020 at 14:24, Stam Markianos-Wright via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi all,
>
> A while back I submitted GCC10 commit:
>
>   44f77a6dea2f312ee1743f3dde465c1b8453ee13
>
> for PR91816.
>
> Turns out I was an idiot and forgot to include the test in the actual
> git commit, even my entire patch had been approved.
>
> Tested that the test still passes on a cross arm-none-eabi and also in a
> Cortex A-15 bootstrap with no regressions.
>
> Submitting this as Obvious to gcc-11 and backporting to gcc-10.
>

Hi,

This new test fails when forcing -mcpu=cortex-m3/4/5/7/33:
FAIL: gcc.target/arm/pr91816.c scan-assembler-times beq\\t.L[0-9] 2
FAIL: gcc.target/arm/pr91816.c scan-assembler-times beq\\t.Lbcond[0-9] 1
FAIL: gcc.target/arm/pr91816.c scan-assembler-times bne\\t.L[0-9] 2
FAIL: gcc.target/arm/pr91816.c scan-assembler-times bne\\t.Lbcond[0-9] 1

I didn't check manually what is generated, can you have a look?

Thanks,

Christophe




> Thanks,
> Stam Markianos-Wright
>
> gcc/testsuite/ChangeLog:
>         PR target/91816
>         * gcc.target/arm/pr91816.c: New test.

Re: [committed obvious][arm] Add test that was missing from old commit [PR91816]

[Stam Markianos-Wright]

On 26/11/2020 09:01, Christophe Lyon wrote:
> On Wed, 25 Nov 2020 at 14:24, Stam Markianos-Wright via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> Hi all,
>>
>> A while back I submitted GCC10 commit:
>>
>>    44f77a6dea2f312ee1743f3dde465c1b8453ee13
>>
>> for PR91816.
>>
>> Turns out I was an idiot and forgot to include the test in the actual
>> git commit, even my entire patch had been approved.
>>
>> Tested that the test still passes on a cross arm-none-eabi and also in a
>> Cortex A-15 bootstrap with no regressions.
>>
>> Submitting this as Obvious to gcc-11 and backporting to gcc-10.
>>
> 
> Hi,
> 
> This new test fails when forcing -mcpu=cortex-m3/4/5/7/33:
> FAIL: gcc.target/arm/pr91816.c scan-assembler-times beq\\t.L[0-9] 2
> FAIL: gcc.target/arm/pr91816.c scan-assembler-times beq\\t.Lbcond[0-9] 1
> FAIL: gcc.target/arm/pr91816.c scan-assembler-times bne\\t.L[0-9] 2
> FAIL: gcc.target/arm/pr91816.c scan-assembler-times bne\\t.Lbcond[0-9] 1
> 
> I didn't check manually what is generated, can you have a look?
> 

Oh wow thank you for spotting this!

It looks like the A class target that I had tested had a tendency to 
emit a movw/movt pair, whereas these M class targets would emit a single 
ldr. This resulted in an overall shorter jump for these targets that did 
not trigger the new far-branch code.

The test passes after... doubling it's own size:



  #define HW3    HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2
  #define HW4    HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3
  #define HW5    HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4
+#define HW6    HW5 HW5

  __attribute__((noinline,noclone)) void f1 (int a)
  {
@@ -25,7 +26,7 @@ __attribute__((noinline,noclone)) void f2 (int a)

  __attribute__((noinline,noclone)) void f3 (int a)
  {
-  if (a) { HW5 }
+  if (a) { HW6 }
  }

  __attribute__((noinline,noclone)) void f4 (int a)
@@ -41,7 +42,7 @@ __attribute__((noinline,noclone)) void f5 (int a)

  __attribute__((noinline,noclone)) void f6 (int a)
  {
-  if (a == 1) { HW5 }
+  if (a == 1) { HW6 }
  }

But this does effectively double the compilation time of an already 
quite large test. Would that be ok?

Overall this is the edge case testing that the compiler behaves 
correctly with a branch in huge compilation unit, so it would be nice to 
have test coverage of it on as many targets as possible... but also 
kinda rare.

Hope this helps!

Cheers,
Stam



> Thanks,
> 
> Christophe
> 
> 
> 
> 
>> Thanks,
>> Stam Markianos-Wright
>>
>> gcc/testsuite/ChangeLog:
>>          PR target/91816
>>          * gcc.target/arm/pr91816.c: New test.

Re: [committed obvious][arm] Add test that was missing from old commit [PR91816]

[Christophe Lyon]

On Fri, 27 Nov 2020 at 02:03, Stam Markianos-Wright
<stam.markianos-wright@arm.com> wrote:
>
> On 26/11/2020 09:01, Christophe Lyon wrote:
> > On Wed, 25 Nov 2020 at 14:24, Stam Markianos-Wright via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> Hi all,
> >>
> >> A while back I submitted GCC10 commit:
> >>
> >>    44f77a6dea2f312ee1743f3dde465c1b8453ee13
> >>
> >> for PR91816.
> >>
> >> Turns out I was an idiot and forgot to include the test in the actual
> >> git commit, even my entire patch had been approved.
> >>
> >> Tested that the test still passes on a cross arm-none-eabi and also in a
> >> Cortex A-15 bootstrap with no regressions.
> >>
> >> Submitting this as Obvious to gcc-11 and backporting to gcc-10.
> >>
> >
> > Hi,
> >
> > This new test fails when forcing -mcpu=cortex-m3/4/5/7/33:
> > FAIL: gcc.target/arm/pr91816.c scan-assembler-times beq\\t.L[0-9] 2
> > FAIL: gcc.target/arm/pr91816.c scan-assembler-times beq\\t.Lbcond[0-9] 1
> > FAIL: gcc.target/arm/pr91816.c scan-assembler-times bne\\t.L[0-9] 2
> > FAIL: gcc.target/arm/pr91816.c scan-assembler-times bne\\t.Lbcond[0-9] 1
> >
> > I didn't check manually what is generated, can you have a look?
> >
>
> Oh wow thank you for spotting this!
>
> It looks like the A class target that I had tested had a tendency to
> emit a movw/movt pair, whereas these M class targets would emit a single
> ldr. This resulted in an overall shorter jump for these targets that did
> not trigger the new far-branch code.
>
> The test passes after... doubling it's own size:
>
>
>
>   #define HW3    HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2 HW2
>   #define HW4    HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3 HW3
>   #define HW5    HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4 HW4
> +#define HW6    HW5 HW5
>
>   __attribute__((noinline,noclone)) void f1 (int a)
>   {
> @@ -25,7 +26,7 @@ __attribute__((noinline,noclone)) void f2 (int a)
>
>   __attribute__((noinline,noclone)) void f3 (int a)
>   {
> -  if (a) { HW5 }
> +  if (a) { HW6 }
>   }
>
>   __attribute__((noinline,noclone)) void f4 (int a)
> @@ -41,7 +42,7 @@ __attribute__((noinline,noclone)) void f5 (int a)
>
>   __attribute__((noinline,noclone)) void f6 (int a)
>   {
> -  if (a == 1) { HW5 }
> +  if (a == 1) { HW6 }
>   }
>
> But this does effectively double the compilation time of an already
> quite large test. Would that be ok?

I guess that's OK for me.
We can probably increase the timeout value if needed.

>
> Overall this is the edge case testing that the compiler behaves
> correctly with a branch in huge compilation unit, so it would be nice to
> have test coverage of it on as many targets as possible... but also
> kinda rare.
>
> Hope this helps!
>
> Cheers,
> Stam
>
>
>
> > Thanks,
> >
> > Christophe
> >
> >
> >
> >
> >> Thanks,
> >> Stam Markianos-Wright
> >>
> >> gcc/testsuite/ChangeLog:
> >>          PR target/91816
> >>          * gcc.target/arm/pr91816.c: New test.
>