Re: [Patch, AVR]: Fix PR46779

[Georg-Johann Lay <avr@gjlay.de>]

[In CCing Richard Henderson]

Denis Chertykov schrieb:
> 2011/6/10 Georg-Johann Lay <avr@gjlay.de>:
> 
>>Then I have a question on spill failures:
>>
>>There is PR46278, an optimization flaw that goes as follows:
>>
>>The avr BE defines fake addressing mode X+const that has to be written
>>down in asm as
>> X += const
>> a = *X
>> X -= const
>>
>>The comment says that this is just to cover a corner case, however the
>>new register allocator uses this case more often or even greedily.
>>There is no way to describe the cost for such an access, and as X has
>>lower prologue/epilogue cost than Y, X is preferred over Y often.
>>
>>In 4.7, I see that flaw less often than in 4.5.  However, I think the
>>best way is not to lie at the register allocator and not to supply a
>>fake instruction like that.
>>
>>So I started to work on a fix. The changes in avr.h are:
>>
>>       * config/avr/avr.h (BASE_REG_CLASS): Remove.
>>       (REG_OK_FOR_BASE_NOSTRICT_P): Remove.
>>       (REG_OK_FOR_BASE_STRICT_P): Remove.
>>       (MODE_CODE_BASE_REG_CLASS): New Define.
>>       (REGNO_MODE_CODE_OK_FOR_BASE_P): New Define.
>>
>>The macros REGNO_MODE_CODE_OK_FOR_BASE_P and MODE_CODE_BASE_REG_CLASS
>>allow a better, fine grained control over addressing modes for each
>>hard register and allow to support X without fake instructions. The
>>code quality actually improves, but I see a new spill failure for the
>>test case
>>
>>* gcc.c-torture/compile/950612-1.c
>>
>>On the one hand, that test case has heavy long long use and is no
>>"real world code" to run on AVR. On the other hand, I don't think
>>trading code quality here against ICE there is a good idea.
>>
>>What do you think about that? As I have no idea how to fix a spill
>>failure in the backend, I stopped working on a patch.
> 
> Ohhh. It's a most complicated case for GCC for AVR.

So you think is is pointless/discouraged to give a more realistic 
description of AVR addressing be means of MODE_CODE_BASE_REG_CLASS 
(instead of BASE_REG_CLASS) resp. REGNO_MODE_CODE_OK_FOR_BASE_P?

> Look carefully at `out_movqi_r_mr'.
> There are even two fake addressing modes:
> 1. [Y + infinite-dslacement];
> 2. [X + (0...63)].

Yes, I know. The first is introduced by avr_legitimate_address_p and the 
second appears to be artifact of LEGITIMIZE_RELOAD_ADDRESS.

The changes are basically MODE_CODE_BASE_REG_CLASS (introduced in 4.2) 
and a rewrite of avr_legitimate_address_p. The changes aim at a better 
addressing for X and to minimize fake addresses.

> I have spent a many hours (days, months) to debug GCC (especially avr port
> and reload) for right addressing modes.
> I have stopped on this code.
> AVR have a limited memory addressing and GCC can't handle it in native form.
> Because of that I have supported a fake adddressing modes.

I assume the code is from prior to 4.2 when 
REGNO_MODE_CODE_OK_FOR_BASE_P and MODE_CODE_BASE_REG_CLASS had not been 
available so that supporting X required some hacking.
All that would still be fine; however the new register allocator leads 
to code that noone would accept. Accessing a structure through a pointer 
is not uncommon, not even on AVR. So if Z is used for, say accessing 
flash, X appears to be the best register.

The shortcoming in GCC is that there is no way to give costs of 
addressing (TARGET_ADDRESS_COST does different things).

So take a look what avr-gcc compiles here:
   http://gcc.gnu.org/bugzilla/attachment.cgi?id=22242
I saw similar complains in forums on the web.

> (Richard Henderson have a different opinion: GCC can, AVR port can't)

What does he mean with that?

> IMHO that three limited pointer registers is not enough for C compiler.
> Even more with frame pointer it's only two and X is a very limited.

The current implementation has several oddities like

* allowing SUBREGs in avr-legitimate_address_p
* changing BASE_REG_CLASS on the fly (by means of reload_completed)

isn't that supposed to cause trouble?

> 1. [Y + infinite-dslacement] it's helper for reload addressing.
> For addressing too long local/spilled variable. (Y + more-than-63)
> 
> 2. [X + (0...63)] for another one "normal" pointer address.

____________________

>>Then I observed trouble with DI patterns during libgcc build and had
>>to remove
>>
>>* "zero_extendqidi2"
>>* "zero_extendhidi2"
>>* "zero_extendsidi2"
>>
>>These are "orphan" insns: they deal with DI without having movdi
>>support so I removed them.
> 
> This seems strange for me.

As far as I know, to support a mode a respective mov insn is needed, 
which is not the case for DI. I don't know the exact rationale behind 
that (reloading?), just read is on gcc list by Ian Taylor (and also that 
it is stronly discouraged to have more than one mov insn per mode).

So if the requirement to have mov insn is dropped and without the burden 
to implement movdi, it would be rather easy to implement adddi3 and 
subdi3 for avr...

Johann

> Denis.