Re: [Patch, avr] Relax LDS/STS to IN/OUT if symbol is in I/O address range

[Senthil Kumar Selvaraj <senthil_kumar dot selvaraj at atmel dot com>]

On Mon, Oct 06, 2014 at 09:59:46AM +0530, Senthil Kumar Selvaraj wrote:
> On Mon, Oct 06, 2014 at 12:33:48AM +0200, Georg-Johann Lay wrote:
> > Senthil Kumar Selvaraj schrieb:
> > >On Mon, Sep 29, 2014 at 08:08:37PM +0400, Denis Chertykov wrote:
> > >>2014-09-29 14:45 GMT+04:00 Senthil Kumar Selvaraj
> > >>>The patch below adds linker relaxation support for rewriting LDS/STS
> > >>>instructions to IN/OUT where appropriate. The IN/OUT instruction is
> > >>>shorter by a couple of bytes, and executes a cycle quicker. The
> > >>>compiler already does this optimization for addresses known at
> > >>>compile time - this linker patch does it for the rest.
> > >>>
> > >>>The patch looks for R_AVR_16 relocations, and if it finds an
> > >>>LDS/STS instruction with the symbol value (i.e. address) in I/O range,
> > >>>rewrites it to use IN/OUT instead, adjusting the address for SFR offsets.
> > >>>
> > >>>The patch also includes a couple of test cases to verify that it works
> > >>>for tiny, mega and xmega archs, and to ensure I/O range check is
> > >>>implemented correctly.
> > >>>
> > >>>If ok, could someone commit please? I don't have commit access.
> > >>Generally, I don't like this.
> > >>The compiler already does this optimization.
> > >>May be somebody want to have LDS/STS instead of IN/OUT.
> > >>(Calc delay)
> > >
> > >Hmm, but can't you make that argument for all linker relaxations (jmp ->
> > >rjmp, for e.g.)? Besides, this relaxation kicks in only if there is a
> > >relocation - a plain lds <register> <constant_address> won't get rewritten
> > >unless a .reloc directive is used to forcibly emit a relocation. And of
> > >course, the user can simply choose to NOT pass --relax to the linker.
> > >
> > >The reason this patch came about was some (inline assembly) code in
> > >avr-libc (wdt.h) that has conditional compilation branches for emitting
> > >LDS/STS vs IN/OUT based on the device name. We figured it would be simpler
> > >to let the linker deal with it instead.
> > 
> > Instead of mentioning each and every device in #if defined(), did you
> > consider using an always inline function and just write
> > 
> >   if (addr < A0)
> >     {
> >        __asm __volatile__ (...
> >     }
> >   else if (addr < A1)
> >     {
> >        __asm __volatile__ (...
> >     }
> > 
> > GCC kicks out the dead alternatives even at -O0 because both addr and A0
> > etc. are known at compile time.
> > 
> > You'll still have to write down all possible OUT / STS combinations, but
> > once you've accomplished that there's no need to touch wdt.h for new devices
> > -- except in the case when the device needs a complete different scheme for
> > WDT which means you have to extend wdt.h anyway...
> > 
> 
> Sounds neat - I did not think of this. Makes sense because the code in
> the header is intended to be directly included - user code does not link
> against it.
> 
> I messed about with macros to see if I can use the
> preprocessor to make the branching decision, but I couldn't "unwrap" the raw
> address out of the _SFR_{IO/MEM}_ADDR macro.
> 
> Thanks Johann - I'll give this a try.
> 

That worked, and I've sent a patch to the avr-libc mailing list.

That said, this patch will still help if the address read from/written
to is not a compile time constant and happens to fall in IO address
range (extern variables, library code etc..).

According to the binutils documentation
(https://sourceware.org/binutils/docs/ld/M68HC11_002f68HC12.html#M68HC11_002f68HC12)
the linker even transforms addressing modes. This patch does something
similar.

What other downsides do you guys see, except for higher link times with
--relax?

Regards
Senthil