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

[Senthil Kumar Selvaraj]

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.

Regards
Senthil