input address reload issue

input address reload issue

[Aurelien Buhrig]

Hi all, and best wishes for the happy new year!

I'm porting a private 4.6 backend to GCC 6 and facing a reload issue and
I would appreciate a little help to cope with it.

The issue happens when reloading:

(set (reg:QI 47 [ _9 ])
  (mem:QI (plus:SI (reg/v/f:SI 68 [orig:51 in ] [51])
                (const_int 1 [0x1])

My understanding is that IRA allocates hardregs to allocno which are
replaced by the reload pass which generates new reloads or spills regs
when needed, right?

Here the IRA chooses a reg (named r2)which makes the mem address not
legitimate. Is it valid to allocate a reg which makes non legitimate
address?

Assuming it is, my understanding is that the reload chooses a legitimate
reg (named a0 here) and shall emit insns (in emit_reload_insns) to set
a0 correctly (RELOAD_FOR_INPUT_ADDRESS). Right?

So the insn:
(set (reg:QI 0 r0) (mem:QI (plus:SI (reg:SI 2 r2)(const_int 1))

is transformed into:
(set (reg:SI 8 a0) (reg:SI 2 r2))
(set (reg:SI 8 a0) (const_int 1))
(set (reg:SI 8 a0) (plus:SI (reg:SI 8 a0) (reg:SI 8 a0)))
(set (reg:QI 0 r0) (mem:QI (reg:SI 8 a0))

This "basic" transformation requires two reload regs, but only one is
given/used/possible from the rl structure (in emit_reload_insns).

So where does the issue comes from? The need for 2 reload regs, the
transformation which is too "basic" and could be optimized to use only
one reload reg, or any wrong/missing reload target hook?

Thanks in advance.
Aurélien

Re: input address reload issue

[Jeff Law]

On 01/05/2017 09:18 AM, Aurelien Buhrig wrote:
> Hi all, and best wishes for the happy new year!
>
> I'm porting a private 4.6 backend to GCC 6 and facing a reload issue and
> I would appreciate a little help to cope with it.
>
> The issue happens when reloading:
>
> (set (reg:QI 47 [ _9 ])
>   (mem:QI (plus:SI (reg/v/f:SI 68 [orig:51 in ] [51])
>                 (const_int 1 [0x1])
>
> My understanding is that IRA allocates hardregs to allocno which are
> replaced by the reload pass which generates new reloads or spills regs
> when needed, right?
>
> Here the IRA chooses a reg (named r2)which makes the mem address not
> legitimate. Is it valid to allocate a reg which makes non legitimate
> address?
>
> Assuming it is, my understanding is that the reload chooses a legitimate
> reg (named a0 here) and shall emit insns (in emit_reload_insns) to set
> a0 correctly (RELOAD_FOR_INPUT_ADDRESS). Right?
>
> So the insn:
> (set (reg:QI 0 r0) (mem:QI (plus:SI (reg:SI 2 r2)(const_int 1))
>
> is transformed into:
> (set (reg:SI 8 a0) (reg:SI 2 r2))
> (set (reg:SI 8 a0) (const_int 1))
> (set (reg:SI 8 a0) (plus:SI (reg:SI 8 a0) (reg:SI 8 a0)))
> (set (reg:QI 0 r0) (mem:QI (reg:SI 8 a0))
>
> This "basic" transformation requires two reload regs, but only one is
> given/used/possible from the rl structure (in emit_reload_insns).
>
> So where does the issue comes from? The need for 2 reload regs, the
> transformation which is too "basic" and could be optimized to use only
> one reload reg, or any wrong/missing reload target hook?
Sounds like you need secondary or intermediate reloads.

Alternately, convert your port to LRA.  It probably will still require 
some work, but LRA (rather than reload) is where the team focuses their 
energy these days.

jeff

Re: input address reload issue

[Aurelien Buhrig]

>> So the insn:
>> (set (reg:QI 0 r0) (mem:QI (plus:SI (reg:SI 2 r2)(const_int 1))
>>
>> is transformed into:
>> (set (reg:SI 8 a0) (reg:SI 2 r2))
>> (set (reg:SI 8 a0) (const_int 1))
>> (set (reg:SI 8 a0) (plus:SI (reg:SI 8 a0) (reg:SI 8 a0)))
>> (set (reg:QI 0 r0) (mem:QI (reg:SI 8 a0))
>>
>> This "basic" transformation requires two reload regs, but only one is
>> given/used/possible from the rl structure (in emit_reload_insns).
>>
>> So where does the issue comes from? The need for 2 reload regs, the
>> transformation which is too "basic" and could be optimized to use only
>> one reload reg, or any wrong/missing reload target hook?
> Sounds like you need secondary or intermediate reloads.
Thank you Jeff for your help.
Currently, the TARGET_SECONDARY_RELOAD does not check if the address of
a mem rtx is legitimate address and it returns NO_REG with (mem:QI r2).
Do you suggest the secondary reload must implement a scratch reg & md
pattern to implement this reload?

Re: input address reload issue

[Jeff Law]

On 01/06/2017 03:20 AM, Aurelien Buhrig wrote:
>
>>> So the insn:
>>> (set (reg:QI 0 r0) (mem:QI (plus:SI (reg:SI 2 r2)(const_int 1))
>>>
>>> is transformed into:
>>> (set (reg:SI 8 a0) (reg:SI 2 r2))
>>> (set (reg:SI 8 a0) (const_int 1))
>>> (set (reg:SI 8 a0) (plus:SI (reg:SI 8 a0) (reg:SI 8 a0)))
>>> (set (reg:QI 0 r0) (mem:QI (reg:SI 8 a0))
>>>
>>> This "basic" transformation requires two reload regs, but only one is
>>> given/used/possible from the rl structure (in emit_reload_insns).
>>>
>>> So where does the issue comes from? The need for 2 reload regs, the
>>> transformation which is too "basic" and could be optimized to use only
>>> one reload reg, or any wrong/missing reload target hook?
>> Sounds like you need secondary or intermediate reloads.
> Thank you Jeff for your help.
> Currently, the TARGET_SECONDARY_RELOAD does not check if the address of
> a mem rtx is legitimate address and it returns NO_REG with (mem:QI r2).
So first you have to distinguish between an intermediate and scratch 
register.

Intermediates say that to copy from a particular register class to a 
particular destination will require the source to first be copied into 
the intermediate register, then the intermediate to the final 
destination.   So for example if you have a register that you can not 
directly store to memory, you might use an intermediate.  The source 
register would be copied to the intermediate and the intermediate then 
stored to memory.

Scratch registers are different -- essentially the backend tells reload 
that another register from a particular class is needed *and* the 
pattern to use for generating the reload.  So as an example, you might 
have a two-register indexed address,

For example, loading a constant into an FP register during PIC code 
generation may require a scratch register to hold intermediate address 
computations.

You can have cases where you need both a scratch and an intermediate. 
You can also have cases where you need an intermediate memory location.


> Do you suggest the secondary reload must implement a scratch reg & md
> pattern to implement this reload?
Perhaps.  I don't know enough about your architecture to be 100% sure 
about how all the pieces interact with each other -- reload, and 
secondary reloads in particular are a complex area.  I'm largely going 
on your comment that you need 2 reload registers.

Presumably you don't have an instruction for
(set (reg) (plus (reg) (const_int)))

Thus you need two scratch reload regs IIUC.  One to hold r2 another to 
hold (const_int 1).  So you'd want to generate

(set (areg1) (reg r2))
(set (areg2) (const_int 1))
(set (areg1) (plus (areg1) (areg2)
(set (r0) (mem (areg1))

Or something along those lines.  If you're going to stick with reload, 
you'll likely want to dig into find_reloads_address and its children to 
see what reloads it generates and why (debug_reload can be helpful here).

jeff

Re: input address reload issue

[Aurelien Buhrig]

On 06/01/2017 17:06, Jeff Law wrote:
> On 01/06/2017 03:20 AM, Aurelien Buhrig wrote:
>>
>>>> So the insn:
>>>> (set (reg:QI 0 r0) (mem:QI (plus:SI (reg:SI 2 r2)(const_int 1))
>>>>
>>>> is transformed into:
>>>> (set (reg:SI 8 a0) (reg:SI 2 r2))
>>>> (set (reg:SI 8 a0) (const_int 1))
>>>> (set (reg:SI 8 a0) (plus:SI (reg:SI 8 a0) (reg:SI 8 a0)))
>>>> (set (reg:QI 0 r0) (mem:QI (reg:SI 8 a0))
>>>>
>>>> This "basic" transformation requires two reload regs, but only one is
>>>> given/used/possible from the rl structure (in emit_reload_insns).
>>>>
>>>> So where does the issue comes from? The need for 2 reload regs, the
>>>> transformation which is too "basic" and could be optimized to use only
>>>> one reload reg, or any wrong/missing reload target hook?
>>> Sounds like you need secondary or intermediate reloads.
>> Do you suggest the secondary reload must implement a scratch reg & md
>> pattern to implement this reload?
> Perhaps.  I don't know enough about your architecture to be 100% sure
> about how all the pieces interact with each other -- reload, and
> secondary reloads in particular are a complex area.  I'm largely going
> on your comment that you need 2 reload registers.
>
> Presumably you don't have an instruction for
> (set (reg) (plus (reg) (const_int)))
>
> Thus you need two scratch reload regs IIUC.  One to hold r2 another to
> hold (const_int 1).  So you'd want to generate
>
> (set (areg1) (reg r2))
> (set (areg2) (const_int 1))
> (set (areg1) (plus (areg1) (areg2)
> (set (r0) (mem (areg1))
>
> Or something along those lines.  If you're going to stick with reload,
> you'll likely want to dig into find_reloads_address and its children
> to see what reloads it generates and why (debug_reload can be helpful
> here). 

Thank you very much Jeff for your help. The best mapping for my target
would be:
(set areg1 r2) (set r0 (mem:QI (areg1 + 1)), and only 1 scratch would be
needed.

In secondary_reload, I can see the pattern (mem:QI (reg+1)) during IRA
pass with reg being a pseudo (ira_cost), but I cannot tranform it since
I do not know in which class the address reg would be reloaded into (or
maybe I can, but I don't know how since reg_renumber is -1 for this pseudo).
And the reload pass does not call my secondary reload with this pattern
mapped onto hw regs, so I cannot transform it.

So I try to add LEGITIMIZE_RELOAD_ADDRESS. I can see the (mem:QI (r2+1))
with hwreg, and so try to push_reload r2 with the "BASE_REG_CLASS".
But it has no effect. Indeed, in push_secondary_reload, the
secondary_reload is called and if NO_REG and CODE_FOR_nothing is
returned, "no reload reg is needed" ... But r2 is a hwreg which does not
belong to BASE_REG_CLASS, can be directly copied to BASE_REG_CLASS and
it does need a reload reg.
So should not push_secondary_reload check for class intersection ? Or
how to perform a reload from a hwreg to a class it does not belong to?

Thanks,
Aurélien

Re: input address reload issue

[Jeff Law]

On 01/09/2017 07:02 AM, Aurelien Buhrig wrote:
> On 06/01/2017 17:06, Jeff Law wrote:
>> On 01/06/2017 03:20 AM, Aurelien Buhrig wrote:
>>>
>>>>> So the insn:
>>>>> (set (reg:QI 0 r0) (mem:QI (plus:SI (reg:SI 2 r2)(const_int 1))
>>>>>
>>>>> is transformed into:
>>>>> (set (reg:SI 8 a0) (reg:SI 2 r2))
>>>>> (set (reg:SI 8 a0) (const_int 1))
>>>>> (set (reg:SI 8 a0) (plus:SI (reg:SI 8 a0) (reg:SI 8 a0)))
>>>>> (set (reg:QI 0 r0) (mem:QI (reg:SI 8 a0))
>>>>>
>>>>> This "basic" transformation requires two reload regs, but only one is
>>>>> given/used/possible from the rl structure (in emit_reload_insns).
>>>>>
>>>>> So where does the issue comes from? The need for 2 reload regs, the
>>>>> transformation which is too "basic" and could be optimized to use only
>>>>> one reload reg, or any wrong/missing reload target hook?
>>>> Sounds like you need secondary or intermediate reloads.
>>> Do you suggest the secondary reload must implement a scratch reg & md
>>> pattern to implement this reload?
>> Perhaps.  I don't know enough about your architecture to be 100% sure
>> about how all the pieces interact with each other -- reload, and
>> secondary reloads in particular are a complex area.  I'm largely going
>> on your comment that you need 2 reload registers.
>>
>> Presumably you don't have an instruction for
>> (set (reg) (plus (reg) (const_int)))
>>
>> Thus you need two scratch reload regs IIUC.  One to hold r2 another to
>> hold (const_int 1).  So you'd want to generate
>>
>> (set (areg1) (reg r2))
>> (set (areg2) (const_int 1))
>> (set (areg1) (plus (areg1) (areg2)
>> (set (r0) (mem (areg1))
>>
>> Or something along those lines.  If you're going to stick with reload,
>> you'll likely want to dig into find_reloads_address and its children
>> to see what reloads it generates and why (debug_reload can be helpful
>> here).
>
> Thank you very much Jeff for your help. The best mapping for my target
> would be:
> (set areg1 r2) (set r0 (mem:QI (areg1 + 1)), and only 1 scratch would be
> needed.
So if you've got reg+d addressing modes, then I'd start by looking at 
your GO_IF_LEGITIMATE_ADDRESS, REG_OK_FOR_*_P, etc.  Reload ought to be 
able to reload r2 into an address register without defining secondary 
reloads.  From what you've described, it's comparable to any target that 
has split address/data registers.
>
> So I try to add LEGITIMIZE_RELOAD_ADDRESS. I can see the (mem:QI (r2+1))
> with hwreg, and so try to push_reload r2 with the "BASE_REG_CLASS".
> But it has no effect. Indeed, in push_secondary_reload, the
> secondary_reload is called and if NO_REG and CODE_FOR_nothing is
> returned, "no reload reg is needed" ... But r2 is a hwreg which does not
> belong to BASE_REG_CLASS, can be directly copied to BASE_REG_CLASS and
> it does need a reload reg.
Never use LEGITIMIZE_RELOAD_ADDRESS to fix a correctness issue.  It is 
strictly a way to improve the generated code.  It's primary purpose is 
to allow improvement of reg+d addresses when the displacement is out of 
range.

Jeff

Re: input address reload issue

[Aurelien Buhrig]

On 09/01/2017 19:35, Jeff Law wrote:
> On 01/09/2017 07:02 AM, Aurelien Buhrig wrote:
>> On 06/01/2017 17:06, Jeff Law wrote:
>>> On 01/06/2017 03:20 AM, Aurelien Buhrig wrote:
>>>>
>>>>>> So the insn:
>>>>>> (set (reg:QI 0 r0) (mem:QI (plus:SI (reg:SI 2 r2)(const_int 1))
>>>>>>
>>>>>> is transformed into:
>>>>>> (set (reg:SI 8 a0) (reg:SI 2 r2))
>>>>>> (set (reg:SI 8 a0) (const_int 1))
>>>>>> (set (reg:SI 8 a0) (plus:SI (reg:SI 8 a0) (reg:SI 8 a0)))
>>>>>> (set (reg:QI 0 r0) (mem:QI (reg:SI 8 a0))
>>>>>>
>>>>>> This "basic" transformation requires two reload regs, but only
>>>>>> one is
>>>>>> given/used/possible from the rl structure (in emit_reload_insns).
>>>>>>
>>>>>> So where does the issue comes from? The need for 2 reload regs, the
>>>>>> transformation which is too "basic" and could be optimized to use
>>>>>> only
>>>>>> one reload reg, or any wrong/missing reload target hook?
>>>>> Sounds like you need secondary or intermediate reloads.
>>>> Do you suggest the secondary reload must implement a scratch reg & md
>>>> pattern to implement this reload?
>>> Perhaps.  I don't know enough about your architecture to be 100% sure
>>> about how all the pieces interact with each other -- reload, and
>>> secondary reloads in particular are a complex area.  I'm largely going
>>> on your comment that you need 2 reload registers.
>>>
>>> Presumably you don't have an instruction for
>>> (set (reg) (plus (reg) (const_int)))
>>>
>>> Thus you need two scratch reload regs IIUC.  One to hold r2 another to
>>> hold (const_int 1).  So you'd want to generate
>>>
>>> (set (areg1) (reg r2))
>>> (set (areg2) (const_int 1))
>>> (set (areg1) (plus (areg1) (areg2)
>>> (set (r0) (mem (areg1))
>>>
>>> Or something along those lines.  If you're going to stick with reload,
>>> you'll likely want to dig into find_reloads_address and its children
>>> to see what reloads it generates and why (debug_reload can be helpful
>>> here).
>>
>> Thank you very much Jeff for your help. The best mapping for my target
>> would be:
>> (set areg1 r2) (set r0 (mem:QI (areg1 + 1)), and only 1 scratch would be
>> needed.
> So if you've got reg+d addressing modes, then I'd start by looking at
> your GO_IF_LEGITIMATE_ADDRESS, REG_OK_FOR_*_P, etc.  Reload ought to
> be able to reload r2 into an address register without defining
> secondary reloads.  From what you've described, it's comparable to any
> target that has split address/data registers.
TARGET_LEGITIMATE_ADDRESS and REGNO_OK_FOR_BASE_P are OK. After trying
to debug emit_input_reload, I decided to give LRA a try (and remove cc0).
This fixes my reload issue !

Thanks for your help and advice.
Aurélien

Re: input address reload issue

[Segher Boessenkool]

On Thu, Jan 05, 2017 at 05:18:46PM +0100, Aurelien Buhrig wrote:
> The issue happens when reloading:
> 
> (set (reg:QI 47 [ _9 ])
>   (mem:QI (plus:SI (reg/v/f:SI 68 [orig:51 in ] [51])
>                 (const_int 1 [0x1])
> 
> My understanding is that IRA allocates hardregs to allocno which are
> replaced by the reload pass which generates new reloads or spills regs
> when needed, right?

Yes.  IRA chooses what hard registers to us where, or memory (i.e. no
register) if that seems best (for example if it ran out of registers).
Then afterwards reload / LRA fixes up everything that isn't valid.

> Here the IRA chooses a reg (named r2)which makes the mem address not
> legitimate. Is it valid to allocate a reg which makes non legitimate
> address?

reload will (or should ;-) ) fix it, but it would of course be better if
IRA could make valid code immediately.

> Assuming it is, my understanding is that the reload chooses a legitimate
> reg (named a0 here) and shall emit insns (in emit_reload_insns) to set
> a0 correctly (RELOAD_FOR_INPUT_ADDRESS). Right?
> 
> So the insn:
> (set (reg:QI 0 r0) (mem:QI (plus:SI (reg:SI 2 r2)(const_int 1))
> 
> is transformed into:
> (set (reg:SI 8 a0) (reg:SI 2 r2))
> (set (reg:SI 8 a0) (const_int 1))
> (set (reg:SI 8 a0) (plus:SI (reg:SI 8 a0) (reg:SI 8 a0)))
> (set (reg:QI 0 r0) (mem:QI (reg:SI 8 a0))

That second instruction kills the result of the first, that is bad.  It
doesn't do anything useful in the first place.  Maybe the first and the
third instructions could be combined as well, or the third and the fourth,
but I don't know your target.

> This "basic" transformation requires two reload regs, but only one is
> given/used/possible from the rl structure (in emit_reload_insns).
> 
> So where does the issue comes from? The need for 2 reload regs, the
> transformation which is too "basic" and could be optimized to use only
> one reload reg, or any wrong/missing reload target hook?

Look at the dump file for reload to see where things come from.  Also
everything Jeff said; you really want LRA.


Segher

Re: input address reload issue

[Aurelien Buhrig]

> Look at the dump file for reload to see where things come from.  Also
> everything Jeff said; you really want LRA.

I will try switching to LRA in a second step, but I think I need first to remove the old cc0...
BTW, in which way the LRA is better than IRA? Is there any benchmarks?

Thanks for your help. 
Aurélien

Re: input address reload issue

[Segher Boessenkool]

On Fri, Jan 06, 2017 at 11:26:40AM +0100, Aurelien Buhrig wrote:
> > Look at the dump file for reload to see where things come from.  Also
> > everything Jeff said; you really want LRA.
> 
> I will try switching to LRA in a second step, but I think I need first to remove the old cc0...

:-)

> BTW, in which way the LRA is better than IRA? Is there any benchmarks?

LRA is a replacement for reload, not for IRA.

LRA already usually creates better performing code than reload does, but
its big advantage is that it is a much better maintainable codebase, so
that we can improve it over time.


Segher

Re: input address reload issue

[Jeff Law]

On 01/06/2017 03:26 AM, Aurelien Buhrig wrote:
>
>> Look at the dump file for reload to see where things come from.  Also
>> everything Jeff said; you really want LRA.
>
> I will try switching to LRA in a second step, but I think I need first to remove the old cc0...
> BTW, in which way the LRA is better than IRA? Is there any benchmarks?
I would suggesting moving away from cc0 first.  cc0 is an abomination 
and should have been abolished years ago -- the only reason is many old 
ports would break and nobody's taken the time to convert them or propose 
them for deprecation.

While we try to keep the cc0-target paths working, they're not exercised 
all that much and can easily break.

jeff

Re: input address reload issue

[Eric Botcazou]

> I would suggesting moving away from cc0 first.  cc0 is an abomination
> and should have been abolished years ago -- the only reason is many old
> ports would break and nobody's taken the time to convert them or propose
> them for deprecation.

It's 8 out of 47 ports, most of them old indeed, with the exception of AVR.
You're probably thinking of m68k, but AVR seems to be more blocking here.

-- 
Eric Botcazou

Re: input address reload issue

[Jeff Law]

On 01/06/2017 09:37 AM, Eric Botcazou wrote:
>> I would suggesting moving away from cc0 first.  cc0 is an abomination
>> and should have been abolished years ago -- the only reason is many old
>> ports would break and nobody's taken the time to convert them or propose
>> them for deprecation.
>
> It's 8 out of 47 ports, most of them old indeed, with the exception of AVR.
> You're probably thinking of m68k, but AVR seems to be more blocking here.
I didn't have any particular port in mind, but yes, of the cc0 ports AVR 
is probably the most active.

jeff