Re: [m68k 11/13] fix symbolic address recognition during reload

[Jeffrey Law <law at redhat dot com>]

On Mon, 2007-02-05 at 23:56 +0100, Roman Zippel wrote:
> Hi,
> 
> On Mon, 5 Feb 2007, Jeffrey Law wrote:
> 
> > > The additional offset is generated later by converting the SI argument to 
> > > a QI argument, but I have to check the exact details again myself.
> > > Note that all mentioned bugzilla references are different bugs.
> > I also wonder if we should be trying to prevent reload from changing
> > the mode of the argument from SI to QI mode -- it seems like we are
> > having numerous problems when arguments get squished down to QImode.
> 
> Actually the argument has already QI mode. This is the reload in question:
> 
> Reloads for insn # 27
> [..]
> Reload 1: reload_in (QI) = (mem/u:QI (plus:SI (reg:SI 13 %a5)
>                                                         (const:SI (plus:SI (symbol_ref:SI ("_Z12LispLessThanR15LispEnvironmenti") [flags 0x41] <function_decl 0xb7906620 LispLessThan>)
>                                                                 (const_int 3 [0x3])))) [0 S1 A8])
>         DATA_REGS, RELOAD_FOR_INPUT (opnum = 1), optional, can't combine
>         reload_in_reg: (subreg:QI (reg:SI 131) 3)
> 
> [..]
> 
> (insn 7 5 9 2 (set (reg:SI 131)
>         (mem/u:SI (plus:SI (reg:SI 13 %a5)
>                 (symbol_ref:SI ("_Z12LispLessThanR15LispEnvironmenti") [flags 0x41] <function_decl 0xb7906620 LispLessThan>)) [0 S4 A8])) 31 {*m68k.md:659} (nil)
>     (expr_list:REG_EQUIV (mem/u:SI (plus:SI (reg:SI 13 %a5)
>                 (symbol_ref:SI ("_Z12LispLessThanR15LispEnvironmenti") [flags 0x41] <function_decl 0xb7906620 LispLessThan>)) [0 S4 A8])
>         (nil)))
> 
> [..]
> 
> (insn 27 23 28 2 (set (mem/s:QI (plus:SI (reg:SI 134)
>                 (const_int 26 [0x1a])) [0 Code_PositiveIntPower+26 S1 A8])
>         (subreg:QI (reg:SI 131) 3)) 38 {*m68k.md:731} (nil)
>     (nil))
> 
> 
> I guess the alternative is to push it into a register, but this actually 
> works fine, the linker simply creates another GOT entry for it, so I don't 
> see why we should disallow it.
OK.  It's not changing the mode of anything, it's simply reloading for
a subreg.  So concerns about mode changes are really a red herring.

ISTM that you don't generally want gunk like <symbol>+offset in the GOT,
instead you'd prefer to just have the raw symbol in the got, then add in
the offset.

That opinion also matches the comments in G_I_L_A which states "When
generating PIC, an address involving a SYMBOL_REF is legitimate if and
only if it is the sum of pic_offset_table_rtx and the SYMBOL_REF" (or
LABEL_REF).

What happens if you twiddle LEGITIMATE_PIC_OPERAND_P instead?  Instead
of having it check for symbolic_operand, instead explicitly check for
just SYMBOL_REF or LABEL_REF which would make it match the comments in
G_I_L_A.  My concern with this approach is how does it affect code
generation earlier in the compiler.

If twiddling LEGITIMATE_PIC_OPERAND_P turns out to be a bad idea, then
we can probably go forward with your patch to INDIRECTABLE_1_ADDRESS_P.
We can then ponder defining LEGITIMIZE_RELOAD_ADDRESS if we want to
twiddle the reloads generated to get better performance or try and
minimize the number of GOT entries if we're inclined to do so.

Jeff