diff mbox series

Ver.2: Add compile option "-msmall-data-limit=0" to avoid using .srodata section for riscv.

Message ID 20221117095355.1928564-1-chenyixuan@iscas.ac.cn
State Accepted
Headers
Series Ver.2: Add compile option "-msmall-data-limit=0" to avoid using .srodata section for riscv. |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

陈逸轩 Nov. 17, 2022, 9:53 a.m. UTC 
  2022-11-17  Yixuan Chen  <chenyixuan@iscas.ac.cn>

        * gcc/testsuite/gcc.dg/pr25521.c: Add compile option "-msmall-data-limit=0" to avoid using .srodata section for riscv.
---
 gcc/testsuite/gcc.dg/pr25521.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jeff Law Nov. 17, 2022, 9:50 p.m. UTC | #1 
On 11/17/22 02:53, Yixuan Chen wrote:
> 2022-11-17  Yixuan Chen  <chenyixuan@iscas.ac.cn>
>
>          * gcc/testsuite/gcc.dg/pr25521.c: Add compile option "-msmall-data-limit=0" to avoid using .srodata section for riscv.
> ---
>   gcc/testsuite/gcc.dg/pr25521.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/testsuite/gcc.dg/pr25521.c b/gcc/testsuite/gcc.dg/pr25521.c
> index 74fe2ae6626..628ddf1a761 100644
> --- a/gcc/testsuite/gcc.dg/pr25521.c
> +++ b/gcc/testsuite/gcc.dg/pr25521.c
> @@ -2,7 +2,8 @@
>      sections.
>   
>      { dg-require-effective-target elf }
> -   { dg-do compile } */
> +   { dg-do compile }
> +   { dg-options "-msmall-data-limit=0" { target { riscv*-*-* } } } */
>   
>   const volatile int foo = 30;
>   

Wouldn't this be better?  It avoids a target specific conditional by 
instead extending what we look for to cover [s]rodata sections.


Thoughts?

Jeff
diff --git a/gcc/testsuite/gcc.dg/pr25521.c b/gcc/testsuite/gcc.dg/pr25521.c
index 74fe2ae6626..63363a03b9f 100644
--- a/gcc/testsuite/gcc.dg/pr25521.c
+++ b/gcc/testsuite/gcc.dg/pr25521.c
@@ -7,4 +7,4 @@
 const volatile int foo = 30;
 
 
-/* { dg-final { scan-assembler "\\.rodata" } } */
+/* { dg-final { scan-assembler "\\.s\?rodata" } } */
Palmer Dabbelt Nov. 17, 2022, 11:59 p.m. UTC | #2 
On Thu, 17 Nov 2022 13:50:00 PST (-0800), gcc-patches@gcc.gnu.org wrote:
>
> On 11/17/22 02:53, Yixuan Chen wrote:
>> 2022-11-17  Yixuan Chen  <chenyixuan@iscas.ac.cn>
>>
>>          * gcc/testsuite/gcc.dg/pr25521.c: Add compile option "-msmall-data-limit=0" to avoid using .srodata section for riscv.
>> ---
>>   gcc/testsuite/gcc.dg/pr25521.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/gcc/testsuite/gcc.dg/pr25521.c b/gcc/testsuite/gcc.dg/pr25521.c
>> index 74fe2ae6626..628ddf1a761 100644
>> --- a/gcc/testsuite/gcc.dg/pr25521.c
>> +++ b/gcc/testsuite/gcc.dg/pr25521.c
>> @@ -2,7 +2,8 @@
>>      sections.
>>
>>      { dg-require-effective-target elf }
>> -   { dg-do compile } */
>> +   { dg-do compile }
>> +   { dg-options "-msmall-data-limit=0" { target { riscv*-*-* } } } */
>>
>>   const volatile int foo = 30;
>>
>
> Wouldn't this be better?  It avoids a target specific conditional by
> instead extending what we look for to cover [s]rodata sections.
>
>
> Thoughts?
>
> Jeff
> diff --git a/gcc/testsuite/gcc.dg/pr25521.c b/gcc/testsuite/gcc.dg/pr25521.c
> index 74fe2ae6626..63363a03b9f 100644
> --- a/gcc/testsuite/gcc.dg/pr25521.c
> +++ b/gcc/testsuite/gcc.dg/pr25521.c
> @@ -7,4 +7,4 @@
>  const volatile int foo = 30;
>
>
> -/* { dg-final { scan-assembler "\\.rodata" } } */
> +/* { dg-final { scan-assembler "\\.s\?rodata" } } */

That's how I usually do it for these tests, there's some other targets 
with sdata too so it fixes the test for everyone.  IIRC I said something 
like that in the v1, but sorry if I'm just getting it confused with some 
other patch.

There's a few of these that need to get chased down for every release, 
maybe we should add some sort of DG hepler?  Not sure that'd keep folks 
from matching on .data, though...
Oria Chiuan Nov. 18, 2022, 3:30 a.m. UTC | #3 
Got it, I used to regard this test case as targeting at test if the const
data would use the ".rodata" section.

Palmer Dabbelt <palmer@dabbelt.com> 于2022年11月18日周五 07:59写道：

> On Thu, 17 Nov 2022 13:50:00 PST (-0800), gcc-patches@gcc.gnu.org wrote:
> >
> > On 11/17/22 02:53, Yixuan Chen wrote:
> >> 2022-11-17  Yixuan Chen  <chenyixuan@iscas.ac.cn>
> >>
> >>          * gcc/testsuite/gcc.dg/pr25521.c: Add compile option
> "-msmall-data-limit=0" to avoid using .srodata section for riscv.
> >> ---
> >>   gcc/testsuite/gcc.dg/pr25521.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/gcc/testsuite/gcc.dg/pr25521.c
> b/gcc/testsuite/gcc.dg/pr25521.c
> >> index 74fe2ae6626..628ddf1a761 100644
> >> --- a/gcc/testsuite/gcc.dg/pr25521.c
> >> +++ b/gcc/testsuite/gcc.dg/pr25521.c
> >> @@ -2,7 +2,8 @@
> >>      sections.
> >>
> >>      { dg-require-effective-target elf }
> >> -   { dg-do compile } */
> >> +   { dg-do compile }
> >> +   { dg-options "-msmall-data-limit=0" { target { riscv*-*-* } } } */
> >>
> >>   const volatile int foo = 30;
> >>
> >
> > Wouldn't this be better?  It avoids a target specific conditional by
> > instead extending what we look for to cover [s]rodata sections.
> >
> >
> > Thoughts?
> >
> > Jeff
> > diff --git a/gcc/testsuite/gcc.dg/pr25521.c
> b/gcc/testsuite/gcc.dg/pr25521.c
> > index 74fe2ae6626..63363a03b9f 100644
> > --- a/gcc/testsuite/gcc.dg/pr25521.c
> > +++ b/gcc/testsuite/gcc.dg/pr25521.c
> > @@ -7,4 +7,4 @@
> >  const volatile int foo = 30;
> >
> >
> > -/* { dg-final { scan-assembler "\\.rodata" } } */
> > +/* { dg-final { scan-assembler "\\.s\?rodata" } } */
>
> That's how I usually do it for these tests, there's some other targets
> with sdata too so it fixes the test for everyone.  IIRC I said something
> like that in the v1, but sorry if I'm just getting it confused with some
> other patch.
>
> There's a few of these that need to get chased down for every release,
> maybe we should add some sort of DG hepler?  Not sure that'd keep folks
> from matching on .data, though...
>
Palmer Dabbelt Nov. 18, 2022, 5:02 a.m. UTC | #4 
On Thu, 17 Nov 2022 19:30:23 PST (-0800), oriachiuan@gmail.com wrote:
> Got it, I used to regard this test case as targeting at test if the const
> data would use the ".rodata" section.

Sorry, I'm not quite sure what you're trying to say here.  Here's a dump 
of how I see things:

In some targets (RISC-V and MIPS) there's multiple copies of the 
data/rodata sections, with the small data/rodata ending up in the small 
sections (`.sdata` and `.srodata`).  I've never actually been 100% on 
that being allowed by any spec, but MIPS did it long before RISC-V so I 
figure software is expected to tolerate the oddness.

In RISC-V we use it to try and place as many symbols as possible close 
to GP, so we're more likely to relax to GP-relative addressing 
sequences.  IIRC that's pretty much the same as MIPS, though they have 
slightly different addressing requirements.

For targets that function this way `.srodata` and `.rodata` are 
functionally equivalent (assuming you're not playing any GP tricks to 
relocate, but those are way out of what's supported).  So unless the 
test is trying to dig into performance issues differences between these 
sections, it should just allow code to target either.

>
> Palmer Dabbelt <palmer@dabbelt.com> 于2022年11月18日周五 07:59写道：
>
>> On Thu, 17 Nov 2022 13:50:00 PST (-0800), gcc-patches@gcc.gnu.org wrote:
>> >
>> > On 11/17/22 02:53, Yixuan Chen wrote:
>> >> 2022-11-17  Yixuan Chen  <chenyixuan@iscas.ac.cn>
>> >>
>> >>          * gcc/testsuite/gcc.dg/pr25521.c: Add compile option
>> "-msmall-data-limit=0" to avoid using .srodata section for riscv.
>> >> ---
>> >>   gcc/testsuite/gcc.dg/pr25521.c | 3 ++-
>> >>   1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/gcc/testsuite/gcc.dg/pr25521.c
>> b/gcc/testsuite/gcc.dg/pr25521.c
>> >> index 74fe2ae6626..628ddf1a761 100644
>> >> --- a/gcc/testsuite/gcc.dg/pr25521.c
>> >> +++ b/gcc/testsuite/gcc.dg/pr25521.c
>> >> @@ -2,7 +2,8 @@
>> >>      sections.
>> >>
>> >>      { dg-require-effective-target elf }
>> >> -   { dg-do compile } */
>> >> +   { dg-do compile }
>> >> +   { dg-options "-msmall-data-limit=0" { target { riscv*-*-* } } } */
>> >>
>> >>   const volatile int foo = 30;
>> >>
>> >
>> > Wouldn't this be better?  It avoids a target specific conditional by
>> > instead extending what we look for to cover [s]rodata sections.
>> >
>> >
>> > Thoughts?
>> >
>> > Jeff
>> > diff --git a/gcc/testsuite/gcc.dg/pr25521.c
>> b/gcc/testsuite/gcc.dg/pr25521.c
>> > index 74fe2ae6626..63363a03b9f 100644
>> > --- a/gcc/testsuite/gcc.dg/pr25521.c
>> > +++ b/gcc/testsuite/gcc.dg/pr25521.c
>> > @@ -7,4 +7,4 @@
>> >  const volatile int foo = 30;
>> >
>> >
>> > -/* { dg-final { scan-assembler "\\.rodata" } } */
>> > +/* { dg-final { scan-assembler "\\.s\?rodata" } } */
>>
>> That's how I usually do it for these tests, there's some other targets
>> with sdata too so it fixes the test for everyone.  IIRC I said something
>> like that in the v1, but sorry if I'm just getting it confused with some
>> other patch.
>>
>> There's a few of these that need to get chased down for every release,
>> maybe we should add some sort of DG hepler?  Not sure that'd keep folks
>> from matching on .data, though...
>>
Oria Chiuan Nov. 18, 2022, 5:21 a.m. UTC | #5 
Thank you very much for your patient explanation！

Palmer Dabbelt <palmer@dabbelt.com>于2022年11月18日 周五13:02写道：

> On Thu, 17 Nov 2022 19:30:23 PST (-0800), oriachiuan@gmail.com wrote:
> > Got it, I used to regard this test case as targeting at test if the const
> > data would use the ".rodata" section.
>
> Sorry, I'm not quite sure what you're trying to say here.  Here's a dump
> of how I see things:
>
> In some targets (RISC-V and MIPS) there's multiple copies of the
> data/rodata sections, with the small data/rodata ending up in the small
> sections (`.sdata` and `.srodata`).  I've never actually been 100% on
> that being allowed by any spec, but MIPS did it long before RISC-V so I
> figure software is expected to tolerate the oddness.
>
> In RISC-V we use it to try and place as many symbols as possible close
> to GP, so we're more likely to relax to GP-relative addressing
> sequences.  IIRC that's pretty much the same as MIPS, though they have
> slightly different addressing requirements.
>
> For targets that function this way `.srodata` and `.rodata` are
> functionally equivalent (assuming you're not playing any GP tricks to
> relocate, but those are way out of what's supported).  So unless the
> test is trying to dig into performance issues differences between these
> sections, it should just allow code to target either.
>
> >
> > Palmer Dabbelt <palmer@dabbelt.com> 于2022年11月18日周五 07:59写道：
> >
> >> On Thu, 17 Nov 2022 13:50:00 PST (-0800), gcc-patches@gcc.gnu.org
> wrote:
> >> >
> >> > On 11/17/22 02:53, Yixuan Chen wrote:
> >> >> 2022-11-17  Yixuan Chen  <chenyixuan@iscas.ac.cn>
> >> >>
> >> >>          * gcc/testsuite/gcc.dg/pr25521.c: Add compile option
> >> "-msmall-data-limit=0" to avoid using .srodata section for riscv.
> >> >> ---
> >> >>   gcc/testsuite/gcc.dg/pr25521.c | 3 ++-
> >> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/gcc/testsuite/gcc.dg/pr25521.c
> >> b/gcc/testsuite/gcc.dg/pr25521.c
> >> >> index 74fe2ae6626..628ddf1a761 100644
> >> >> --- a/gcc/testsuite/gcc.dg/pr25521.c
> >> >> +++ b/gcc/testsuite/gcc.dg/pr25521.c
> >> >> @@ -2,7 +2,8 @@
> >> >>      sections.
> >> >>
> >> >>      { dg-require-effective-target elf }
> >> >> -   { dg-do compile } */
> >> >> +   { dg-do compile }
> >> >> +   { dg-options "-msmall-data-limit=0" { target { riscv*-*-* } } }
> */
> >> >>
> >> >>   const volatile int foo = 30;
> >> >>
> >> >
> >> > Wouldn't this be better?  It avoids a target specific conditional by
> >> > instead extending what we look for to cover [s]rodata sections.
> >> >
> >> >
> >> > Thoughts?
> >> >
> >> > Jeff
> >> > diff --git a/gcc/testsuite/gcc.dg/pr25521.c
> >> b/gcc/testsuite/gcc.dg/pr25521.c
> >> > index 74fe2ae6626..63363a03b9f 100644
> >> > --- a/gcc/testsuite/gcc.dg/pr25521.c
> >> > +++ b/gcc/testsuite/gcc.dg/pr25521.c
> >> > @@ -7,4 +7,4 @@
> >> >  const volatile int foo = 30;
> >> >
> >> >
> >> > -/* { dg-final { scan-assembler "\\.rodata" } } */
> >> > +/* { dg-final { scan-assembler "\\.s\?rodata" } } */
> >>
> >> That's how I usually do it for these tests, there's some other targets
> >> with sdata too so it fixes the test for everyone.  IIRC I said something
> >> like that in the v1, but sorry if I'm just getting it confused with some
> >> other patch.
> >>
> >> There's a few of these that need to get chased down for every release,
> >> maybe we should add some sort of DG hepler?  Not sure that'd keep folks
> >> from matching on .data, though...
> >>
>
陈逸轩 Nov. 18, 2022, 5:52 a.m. UTC | #6 
Thank you very much for your example! I have sent a new patch according to your guide.

&quot;Jeff Law&quot; &lt;jeffreyalaw@gmail.com&gt;wrote:
> 
> On 11/17/22 02:53, Yixuan Chen wrote:
> > 2022-11-17  Yixuan Chen  <chenyixuan@iscas.ac.cn>
> >
> >          * gcc/testsuite/gcc.dg/pr25521.c: Add compile option "-msmall-data-limit=0" to avoid using .srodata section for riscv.
> > ---
> >   gcc/testsuite/gcc.dg/pr25521.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/gcc/testsuite/gcc.dg/pr25521.c b/gcc/testsuite/gcc.dg/pr25521.c
> > index 74fe2ae6626..628ddf1a761 100644
> > --- a/gcc/testsuite/gcc.dg/pr25521.c
> > +++ b/gcc/testsuite/gcc.dg/pr25521.c
> > @@ -2,7 +2,8 @@
> >      sections.
> >   
> >      { dg-require-effective-target elf }
> > -   { dg-do compile } */
> > +   { dg-do compile }
> > +   { dg-options "-msmall-data-limit=0" { target { riscv*-*-* } } } */
> >   
> >   const volatile int foo = 30;
> >   
> 
> Wouldn't this be better?  It avoids a target specific conditional by 
> instead extending what we look for to cover [s]rodata sections.
> 
> 
> Thoughts?
> 
> Jeff
diff mbox series

Patch

diff --git a/gcc/testsuite/gcc.dg/pr25521.c b/gcc/testsuite/gcc.dg/pr25521.c
index 74fe2ae6626..628ddf1a761 100644
--- a/gcc/testsuite/gcc.dg/pr25521.c
+++ b/gcc/testsuite/gcc.dg/pr25521.c
@@ -2,7 +2,8 @@ 
    sections.
 
    { dg-require-effective-target elf }
-   { dg-do compile } */
+   { dg-do compile }
+   { dg-options "-msmall-data-limit=0" { target { riscv*-*-* } } } */
 
 const volatile int foo = 30;