* [committed obvious][arm] Add test that was missing from old commit [PR91816]
@ 2020-11-25 13:24 Stam Markianos-Wright
2020-11-26 9:01 ` Christophe Lyon
0 siblings, 1 reply; 4+ messages in thread
From: Stam Markianos-Wright @ 2020-11-25 13:24 UTC (permalink / raw)
To: gcc-patches
[-- 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 } } */
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [committed obvious][arm] Add test that was missing from old commit [PR91816]
2020-11-25 13:24 [committed obvious][arm] Add test that was missing from old commit [PR91816] Stam Markianos-Wright
@ 2020-11-26 9:01 ` Christophe Lyon
2020-11-27 1:03 ` Stam Markianos-Wright
0 siblings, 1 reply; 4+ messages in thread
From: Christophe Lyon @ 2020-11-26 9:01 UTC (permalink / raw)
To: Stam Markianos-Wright; +Cc: gcc-patches
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [committed obvious][arm] Add test that was missing from old commit [PR91816]
2020-11-26 9:01 ` Christophe Lyon
@ 2020-11-27 1:03 ` Stam Markianos-Wright
2020-11-27 9:23 ` Christophe Lyon
0 siblings, 1 reply; 4+ messages in thread
From: Stam Markianos-Wright @ 2020-11-27 1:03 UTC (permalink / raw)
To: Christophe Lyon; +Cc: gcc-patches
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.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [committed obvious][arm] Add test that was missing from old commit [PR91816]
2020-11-27 1:03 ` Stam Markianos-Wright
@ 2020-11-27 9:23 ` Christophe Lyon
0 siblings, 0 replies; 4+ messages in thread
From: Christophe Lyon @ 2020-11-27 9:23 UTC (permalink / raw)
To: Stam Markianos-Wright; +Cc: gcc-patches
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.
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-27 9:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-25 13:24 [committed obvious][arm] Add test that was missing from old commit [PR91816] Stam Markianos-Wright
2020-11-26 9:01 ` Christophe Lyon
2020-11-27 1:03 ` Stam Markianos-Wright
2020-11-27 9:23 ` Christophe Lyon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).