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

[Georg-Johann Lay <avr at gjlay dot de>]

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...

Johann