RE: [CR16] Improper register allocation

["Jayant R. Sonar" <Jayant dot Sonar at kpitcummins dot com>]

Hello Ian,

Thank you for the valuable inputs.

>> Or are you saying that that the instruction
>>         addd	$3, (r1,r0)
>> may modify r1, and therefore must be executed before the push
>> instruction?

No, the sequence of the instructions is correct here.
What is expected here is (r1,r0) should be loaded with an address value 
before being added by 3.

I looked into the RTL dumps and found the original call sequence, where 
(r1,r0) is loaded with appropriate address value before adding 3, is valid 
up to IRA phase. The reload phase is optimizing out the important load 
instruction (movsi_long).

Dump before 'postreload' i.e. at 'ira':
===========================================================================
(insn 1613 2944 2945 65 Convert.c:707 
(set (mem/f/i:SI (pre_dec:SI (reg/f:SI 15 sp)) [0 S4 A32])
        (reg:SI 1 r1)) 61 {pushsi_internal} (nil))

(insn 2945 1613 1614 65 Convert.c:707 
(set (reg/f:SI 0 r0 [1056])
        (reg/f:SI 12 r12 [orig:525 D.18812 ] [525])) 67 {*movsi_long} (nil))

(insn 1614 2945 1615 65 Convert.c:707 
(set (reg/f:SI 0 r0 [1056])
        (plus:SI (reg/f:SI 0 r0 [1056])
            (const_int 3 [0x3]))) 1 {addsi3} (nil))


Dump of 'postreload':
===========================================================================
(insn 1613 2944 1614 65 Convert.c:707 
(set (mem/f/i:SI (pre_dec:SI (reg/f:SI 15 sp)) [0 S4 A32])
        (reg:SI 1 r1)) 61 {pushsi_internal} (nil))

(insn 1614 1613 1615 65 Convert.c:707 
(set (reg/f:SI 0 r0 [1056])
        (plus:SI (reg/f:SI 0 r0 [1056])
            (const_int 3 [0x3]))) 1 {addsi3} (nil))


The instruction 2945 is deleted during reload phase.
This instruction was loading an address into register pair (r1,r0). And next 
instruction 1614, adds an offset value 3 into this address. However, as 2945 
is deleted, (r1,r0) does not receive the expected address value before 3 is 
added into it by 1614.

Just for the reference, giving here again the inappropriate assembly code 
generated:

        addd    (sp), (r2,r1)
        push    $2,r1   <<<<<<<<<<<<<<<<<==================================
.LCFI548:                    Insn loading addr in (r1,r0) should be here
        addd    $3, (r1,r0)  <<<<<<<<<<<<<<<<<<<<==========================

The push command in CR16 is capable of pushing eight adjacent registers onto 
the stack. The count value in the instruction specifies the number of words 
to save.

So ACCUMULATE_OUTGOING_ARGS may not be efficient when multiple pushes are to
 be considered. 

I can not give the complete RTL dump files here. However 2 RTL dumps having 
part of the concerned code are attached with this mail. 

Thanks and Regards,
Jayant Sonar
[KPIT Cummins, Pune]


-----Original Message-----
From: Ian Lance Taylor [mailto:iant@google.com] 
Sent: Saturday, November 19, 2011 10:35 PM
To: Jayant R. Sonar
Cc: gcc-help@gcc.gnu.org
Subject: Re: [CR16] Improper register allocation

"Jayant R. Sonar" <Jayant.Sonar@kpitcummins.com> writes:

> I am working on CR16 port.
>
> Found an improper code is being generated for following part of the test 
> code:
> ============================================================================
> IeData     = (void *) RAlloc(DataLen);
> LAHType* p = (LAHType*)IeData;
>
> ReadFH (DpEntry, &FeData, &p->Table, &p->Masked, &p->NumAH, &p->Len, TData);
> ============================================================================
>
> Arguments are pushed on to the stack from right to left direction.
> Possible working assembly that could be generated for this code:
> Case 1:
> ================================================================
>        bal (ra), _RAlloc@c
>        stord	(r1,r0), 32(sp)
>        addd	$-2, (sp)
> .LCFI547:
>        movd	$18, (r7,r6)
>        addd	(sp), (r7,r6)
>        push	$2,r6
> .LCFI548:
>        addd	$3, (r1,r0)
>        push	$2,r0
>
> OR lesser optimized code could be:
> Case 2:
> ============================================================
>        bal (ra), _RAlloc@c
>        stord	(r1,r0), 32(sp)
>        addd	$-2, (sp)
> .LCFI547:
>        movd	$18, (r7,r6)
>        addd	(sp), (r7,r6)
>        push	$2,r6
> .LCFI548:
>        loadd	32(sp), (r1,r0)
>        addd	$3, (r1,r0)
>        push	$2,r0
>
> However, I am getting following code:
> ============================================================
>        bal (ra), _RAlloc@c
>        stord	(r1,r0), 20(sp)
>        addd	$-2, (sp)
> .LCFI547:
>        movd	$18, (r2,r1)
>        addd	(sp), (r2,r1)
>        push	$2,r1	  
> .LCFI548:
>        addd	$3, (r1,r0)	<<============== PROBLEM
>        push	$2,r0
>
> 'R1' being the part of first push already has some value in it. So before 
> next push, either (r1,r0) should be loaded with new address value as shown 
> in Case 2. Otherwise, as in Case 1, different register pairs should be used 
> for these two push operations.
>
> CR16 port did not have REG_ALLOC_ORDER defined.
> I am trying to control register allocation order using this. However, it 
> looks like this sequence has to be decided very carefully as slightest of 
> the alternation in sequence are causing some or other application failures.
>
> Is there any other way, I could address this problem?


I don't know this syntax.  Are you saying that the instruction
        push	$2,r1
modifies r1?  Or are you saying that that the instruction
        addd	$3, (r1,r0)
may modify r1, and therefore must be executed before the push
instruction?

Either way, REG_ALLOC_ORDER is not an issue here.  You need to look at
the RTL dumps.  See whether the original call sequence in the first RTL
dump is valid, using pseudo-registers.  Then find out where it becomes
invalid.

One thing to consider is whether the push instruction is particularly
efficient on your processor.  If it is not--if it is comparable to a mov
instruction to an offset from sp--then consider defining
ACCUMULATE_OUTGOING_ARGS rather than PUSH_ARGS.  Most processors get
more efficient code from ACCUMULATE_OUTGOING_ARGS.

Ian

Attachment: Convert.c.188r.ira.txt
Description: Convert.c.188r.ira.txt

Attachment: Convert.c.189r.postreload.txt
Description: Convert.c.189r.postreload.txt