Re: [PATCH] RISC-V: Adjust __global_pointer$ value to reduce code size.

[Palmer Dabbelt <palmer at sifive dot com>]

On Mon, 15 Oct 2018 17:39:06 PDT (-0700), Jim Wilson wrote:
> On Mon, Oct 15, 2018 at 4:43 PM Palmer Dabbelt <palmer@sifive.com> wrote:
> > This smells like a bug lurking somewhere.  IIRC we avoided relaxing anything
> > that points to a mergeable segment due to these sorts of constraints, are we
> > just missing something?  I would consider it a bug if users can end up with
> > incorrect relaxations, regardless of where they're targeting the GP.
>
> We avoid relaxation in mergeable and code sections.  In this case the
> problem is rodata which is neither, but immediately follows .text and
> hence variables in rodata can change address when .text shrinks.  We
> probably never tried to relax references to rodata before.  I didn't
> look at trying to handle this in relaxation itself, it was easier to
> just fix gp to avoid overlapping rodata.  We should probably look at
> the rodata problem later though.

Ah, OK.  I guess I just assemed .rodata was mergeable because I assumed strings 
ended up in there.  Either way: it seems like a bug, but only one that you just 
found when writing this patch and not one introduced by this patch.  Certainly 
it shouldn't block a merge.

> > FWIW, when drag racing linker relaxation I always ended up using an offset of
> > 0x7C0 instead of 0x800 because of that extra bit of pessimism in the linker
> > relaxations that eats a few bytes of offset.  IIRC that saves you something
> > like 3 instructions in Dhrystone's inner loop because it allows relaxing
> > against the first few bytes of .sdata.
>
> The current definition is .sdata+0x800 which makes everything .sdata
> and later relaxable.  If you used .sdata+0x7c0, then you would get the
> last 64 bytes of the .data section as relaxable.  My patch should do
> this automatically for you if .sdata+.sbss is smaller than 0x1000.
> Though if you have your own linker script you don't get any benefit
> from this unless you update your linker script.;

Ah, yes, I guess that was the point: while ".sdata+0x800" should allow relaxing 
against a symbol at ".sdata+0", in practice we have a bit of pessimism in the 
relaxation code.  I'm actually afraid I don't understand exactly where it comes 
from, but if you look at "max_alignment" and "reserve_size" in 
"_bfd_riscv_relax_section()" you'll see there's a bit -- 0x7C0 seems a bit 
conservative, I'd guess that you can relax against ".sdata+0" down to 
"__global_pointer$=.sdata+0x7F0".

It actually looks like my misunderstanding of "max_alignment" is the source of 
this auipc+addend bug you're run in to: I appear to have just blindly copied 
this alignment pessimism (IIUC, this shrinks the relaxable region by the symbol 
size, which would avoid the overflows for LUI-based sequences) from the LUI 
relaxations into the AUIPC relaxations.  Now that I actually understand the 
code (or at least, I think I understand it) that seems completely bogus for the 
AUIPC case.

Sorry!

> > I don't know if it matters any more here, as if I understand how this is all
> > working we'll end up with GP pointing quite a way before ".sdata+0x800" for
> > small programs and for big programs the extra few symbols will just be noise.
>
> For big programs there will be no extra symbols, because if
> .sdata+.bss is 0x1000 or larger we get the exact same gp value as
> before.  The smaller the program the more the benefit.  The larger the
> program the less the benefit, with the benefit dropping to zero once
> the program gets big enough.

Yes, I agree -- and I think that's why this is really splitting hairs.  This 
really only matter when drag racing Dhrystone, which isn't worth the time -- 
and also certainly shouldn't block a merge :)