#1148 Bad 'for' loop optimization

closed-fixed
Maarten Brock
5
2013-05-25
2006-06-14
Goofy
No

using "SDCC : pic16 2.5.6 #4199 (May 30 2006)", I have
some troubles with the following for loop :

static uchar i;

for(i = 1; i < 16; i++)
{
ep_init[coming_cfg][i]();
}

This code has been compiled with :

sdcc -mpic16 -p18f4455 -Iinclude -c -c ep0.c

The code generated is the following :

; .line 228; ep0.c for(i = 1; i < 16; i++)
MOVLW 0x0f
BANKSEL _ep0_in_i_3_17
MOVWF _ep0_in_i_3_17, B
; ;multiply lit val:0x03 by variable _coming_cfg and
store in r0x00
; ;Unrolled 8 X 8 multiplication
; ;FIXME: the function does not support result==WREG
_00191_DS_:
BANKSEL _coming_cfg
; .line 230; ep0.c ep_init[coming_cfg][i]();
MOVF _coming_cfg, W, B
MULLW 0x03
MOVFF PRODL, r0x00
MOVFF PRODH, r0x01
BANKSEL _ep_init
MOVF _ep_init, W, B
ADDWF r0x00, F
BANKSEL (_ep_init + 1)
MOVF (_ep_init + 1), W, B
ADDWFC r0x01, F
BANKSEL (_ep_init + 2)
MOVF (_ep_init + 2), W, B
CLRF r0x02
ADDWFC r0x02, F
MOVFF r0x00, FSR0L
MOVFF r0x01, PRODL
MOVF r0x02, W
CALL __gptrget3
MOVWF r0x00
MOVFF PRODL, r0x01
MOVFF PRODH, r0x02
BANKSEL _ep0_in_i_3_17
DECF _ep0_in_i_3_17, W, B
MOVWF r0x03
; ;multiply lit val:0x03 by variable r0x03 and store in
r0x03
; ;Unrolled 8 X 8 multiplication
; ;FIXME: the function does not support result==WREG
MOVF r0x03, W
MULLW 0x03
MOVFF PRODL, r0x03
MOVFF PRODH, r0x04
MOVF r0x03, W
ADDWF r0x00, F
MOVF r0x04, W
ADDWFC r0x01, F
CLRF WREG
ADDWFC r0x02, F
MOVFF r0x00, FSR0L
MOVFF r0x01, PRODL
MOVF r0x02, W
CALL __gptrget3
MOVWF r0x00
MOVFF PRODL, r0x01
MOVFF PRODH, r0x02
MOVFF INTCON, POSTDEC1
BCF INTCON, 7
PUSH
MOVLW LOW(_00210_DS_)
MOVWF TOSL
MOVLW HIGH(_00210_DS_)
MOVWF TOSH
MOVLW UPPER(_00210_DS_)
MOVWF TOSU
MOVF PREINC1, W
MOVWF INTCON
MOVFF r0x02, PCLATU
MOVFF r0x01, PCLATH
MOVF r0x00, W
MOVWF PCL
_00210_DS_:
BANKSEL _ep0_in_i_3_17
DECF _ep0_in_i_3_17, F, B
BANKSEL _ep0_in_i_3_17
; .line 228; ep0.c for(i = 1; i < 16; i++)
MOVF _ep0_in_i_3_17, W, B
BTFSS STATUS, 2
GOTO _00191_DS_

SDCC transforms the loop (on iCode level) from
for (i=1; i < 16; i++) {}
into
for (i=14; i >= 0; i--) {}

which is wrong as `i' is used in the loop body; it
should have been
for (i=15; i >= 1; i--) {}

But even this form is incorrect if the order of the
calls is important.

Discussion

  • Robert Larice
    Robert Larice
    2007-07-22

    Logged In: YES
    user_id=1840151
    Originator: NO

    the problem is in src/SDCCast.c
    funktion isConformingBody (ast * pbody, symbol * sym, ast * body)
    ...
    case CALL:
    /* if local & not passed as paramater then ok */
    if (sym->level && !astHasSymbol(pbody->right,sym))
    return TRUE;
    return FALSE;

    this check should be more restrictive, for example
    && IS_AST_SYM_VALUE(pbody->right)

    Robert Larice

     
  • Robert Larice
    Robert Larice
    2007-07-26

    Logged In: YES
    user_id=1840151
    Originator: NO

    here is a testcase and a suggested patch,

    Robert Larice

    --- sdcc-4870/support/regression/tests/bug1505811.c.orig 1970-01-01 01:00:00.000000000 +0100
    +++ sdcc-4870/support/regression/tests/bug1505811.c 2007-07-26 22:24:41.000000000 +0200
    @@ -0,0 +1,32 @@
    +/** bug 1505811
    + * demonstrates an incorrect "loopreverse"
    + * note func0, ist a kind of saveguard, the incorrect code
    + * will access indices 0 and 1 instead of 1 and 2,
    + * and with incorrect order
    + */
    +
    +#include <testfwk.h>
    +
    +char glbl;
    +
    +void func0() { glbl = 0; }
    +void func1() { glbl = 1; }
    +void func2() { glbl = 2; }
    +
    +typedef void (*fptr)();
    +
    +fptr ep_init[3] = {
    + func0, func1, func2
    +};
    +
    +void buggy() {
    + unsigned char i;
    + for(i = 1; i <= 2; i++)
    + ep_init[i]();
    +}
    +
    +void
    +testBug(void) {
    + buggy();
    + ASSERT(glbl == 2);
    +}

    --- sdcc-4870/src/SDCCast.c.orig 2007-06-28 11:18:43.000000000 +0200
    +++ sdcc-4870/src/SDCCast.c 2007-07-26 22:09:22.000000000 +0200
    @@ -1978,5 +1978,6 @@
    case CALL:
    /* if local & not passed as paramater then ok */
    - if (sym->level && !astHasSymbol(pbody->right,sym))
    + if (sym->level && !astHasSymbol(pbody->right,sym)
    + && IS_AST_SYM_VALUE(pbody->left))
    return TRUE;
    return FALSE;

     
  • Maarten Brock
    Maarten Brock
    2008-03-10

    Logged In: YES
    user_id=888171
    Originator: NO

    Fixed in SDCC #5087.

     
  • Maarten Brock
    Maarten Brock
    2008-03-10

    • milestone: --> fixed
    • assigned_to: nobody --> maartenbrock
    • status: open --> closed-fixed