Hi Claudiu, > I don't think it is a good practice to allow such large offsets (i.e., > scaled) for small data. For one or two large arrays, this may work. > However, having more than that will lead to linker errors. Basically, > your proposed solution doesn't address the core issue. I agree. I considered using some "reasonable" offset. But my intention with this patch is not to address the core issue. So I kept it simple and just allowed up until the maximum allowed offset.
Addressing the core issue doesn't seem worth it right now, especially because by default data only of a couple bytes in size will end up in .sdata. So you either have to manually force large data in .sdata, or do memory access with large, constant, out-of-bounds offsets like in the tests. > - sdata disabled by default in the gcc driver, and enable it on the > user request. I disagree. There is no reason not to have it enabled if it compiles and links. Leaving beneficial features on by default is the best way to ensure users actually adopt it. > - implement a number of optimizations, including linker related that > will convert the loads/store of sdata to regular access. I think this would be the way to go: - Always be pessimistic about offsets in the compiler when referencing data. - Let the linker decide what data goes in .sdata. - Convert ld/st with RISC-V-style relaxations. > - drop the sdata idea, and use pc-relative + anchors, and use gp reg for that. This likely breaks ABI, doesn't sound like an option. Regards, Michiel On 02/12 20:34, Claudiu Zissulescu Ianculescu wrote: > On Fri, Nov 28, 2025 at 2:53 PM Yuriy Kolerov > <[email protected]> wrote: > > > > From: Michiel Derhaeg <[email protected]> > > > > This prevents linker errors when referencing small data using large > > offsets. In practice it is very unlikely to be a real problem but this > > was fixed because other compilers do this check and it ensures the > > following tests now succeed: > > - gcc.dg/torture/pr60115.c > > - gcc.dg/torture/pr105665.c > > > > PR target/115650 > > I don't think it is a good practice to allow such large offsets (i.e., > scaled) for small data. For one or two large arrays, this may work. > However, having more than that will lead to linker errors. Basically, > your proposed solution doesn't address the core issue. > > In my opinion, there are a number of solutions here: > - sdata disabled by default in the gcc driver, and enable it on the > user request. > - implement a number of optimizations, including linker related that > will convert the loads/store of sdata to regular access. Of course, > the conversion of stores is a bit tricky. > - drop the sdata idea, and use pc-relative + anchors, and use gp reg for that. > > Anyhow, I'll allow this patch to go, however, you need to address my > comments before that. > > Cheers, > Claudiu > > > > > gcc/ChangeLog: > > > > * config/arc/arc.cc (legitimate_small_data_address_p): > > Check offset size. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.target/arc/sdata-6.c: New test. > > > > Signed-off-by: Michiel Derhaeg <[email protected]> > > --- > > gcc/config/arc/arc.cc | 24 ++++++++++++++++-------- > > gcc/testsuite/gcc.target/arc/sdata-6.c | 17 +++++++++++++++++ > > 2 files changed, 33 insertions(+), 8 deletions(-) > > create mode 100644 gcc/testsuite/gcc.target/arc/sdata-6.c > > > > diff --git a/gcc/config/arc/arc.cc b/gcc/config/arc/arc.cc > > index 145bc058b09..740b297142b 100644 > > --- a/gcc/config/arc/arc.cc > > +++ b/gcc/config/arc/arc.cc > > @@ -386,27 +386,35 @@ legitimate_small_data_address_p (rtx x, machine_mode > > mode) > > return SYMBOL_REF_SMALL_P (x); > > case PLUS: > > { > > - bool p0 = (GET_CODE (XEXP (x, 0)) == SYMBOL_REF) > > - && SYMBOL_REF_SMALL_P (XEXP (x, 0)); > > + if ((GET_CODE (XEXP (x, 0)) != SYMBOL_REF) > > + || !SYMBOL_REF_SMALL_P (XEXP (x, 0))) > > + return false; > > > > /* If no constant then we cannot do small data. */ > > if (!CONST_INT_P (XEXP (x, 1))) > > return false; > > > > - /* Small data relocs works with scalled addresses, check if > > + const int offset = INTVAL (XEXP (x, 1)); > > + int size = GET_MODE_SIZE (mode); > > + size = size == 8 ? 4 : size; > > + > > + /* Small data relocs works with scaled addresses, check if > > the immediate fits the requirements. */ > > - switch (GET_MODE_SIZE (mode)) > > + switch (size) > > { > > case 1: > > - return p0; > > + break; > > case 2: > > - return p0 && ((INTVAL (XEXP (x, 1)) & 0x1) == 0); > > + if ((offset & 0x1) == 0) break; else return false; > Not GNU standard. > > > case 4: > > - case 8: > > - return p0 && ((INTVAL (XEXP (x, 1)) & 0x3) == 0); > > + if ((offset & 0x3) == 0) break; else return false; > Likewise. > > > default: > > return false; > > } > > + > > + /* Reloc allows scaled signed 9 bits. */ > > + const int v = (offset / size) >> 8; > > + return v == 0 || v == -1; > > } > > default: > > return false; > > diff --git a/gcc/testsuite/gcc.target/arc/sdata-6.c > > b/gcc/testsuite/gcc.target/arc/sdata-6.c > > new file mode 100644 > > index 00000000000..9b58ac54cbe > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/arc/sdata-6.c > > @@ -0,0 +1,17 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-O2 -msdata" } */ > > + > > +__attribute__((section(".sdata"))) int a[300]; > > + > > +int f (void) > > +{ > > + return a[255]; > > +} > > + > > +int g (void) > > +{ > > + return a[256]; > > +} > > + > > +/* { dg-final { scan-assembler "ld_s\\s+r0,\\\[gp,@a@sda\\+1020\\\]" } } */ > > +/* { dg-final { scan-assembler "ld\\s+r0,\\\[@a\\+1024\\\]" } } */ > > I am expecting a load operation using the as flag (i.e., ld.as ), the > best will be to have a test checking the scaling for (8)/16/32 and 64 > loads. > > > -- > > 2.34.1 > >
