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 Thu, Oct 09, 2014 at 06:26:06PM +0400, Denis Chertykov wrote:
> > 2014-10-09 14:34 GMT+04:00 Senthil Kumar Selvaraj
> > <senthil_kumar.selvaraj@atmel.com>:
> > > 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.
> > 1. M68HC1x is not a one of the most popular GCC target. (It's not a
> > good example to follow)
> Oh ok, didn't know that.
> > 2. You poin to a place where the linker change one jump instruction to
> > another. IMHO it's predictable from the linker.
> The AVR linker also performs tail call optimization (call,ret -> jmp)
> > 3. Your patch change loading/storing from memory to input/output -
> > it's not so predictable from the linker.
> Hmm, IMO, it fits with the intent behind --relax
> "On some platforms the `--relax' option performs target specific, global
> optimizations that become possible when the linker resolves addressing
> in the program, such as relaxing address modes, synthesizing new
> instructions, selecting shorter version of current instructions, and
> combining constant values."
> > 4. I worry only on compatibility issues.
> Ok.
> > 5. It's not a strong objection.
> > 6. Look at this (already approved) patch:
> > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg00683.html
>
> Yes, that helps if the user explicitly specifies the attribute on the
> declaration.
> > 6. If somebody else will vote for your patch then it will be approved.
> Ok :)

I must admit that I don't like this kind of "optimization"...

1) There are applications that rely on exact instruction timing,
   e.g. USB-device in software.  If just one instruction has a
   timing other than expected the driver won't work.

2) OUT is not a shorter version of STS:  There was a silicon bug
   in some devices where OUT /STS behaved differently (or IN / LDS).

3) The benefit is limited to very special cases of libraries that
   use I/O where the access instruction is not known at compile time.
  - If you really need performance the library author will ship
    device specific or address range specific version of the library.
  - Generic libraries are not of much use:
    If you could use SBI to set an I/O-Bit, a generic library would
      Store Interrupt State
      Disable Interrupts
      Load Value
      Set Bit
      Store Value
      Restore Interrupt State
    Even if Load / Store can be done as IN / OUT in the end, it's not
    smart to write a library that way...

To solve 1) and 2) you could add a new option for this kind of 
transformation that is off per default.  Turning it on per default or 
adding even more magic to --relax won't help with 1) because the user 
will get a non-functional driver and does not know why it's not working...

Johann