[sdcc-devel] [ sdcc-Bugs-1505811 ] Bad 'for' loop optimization

[SourceForge.net <no...@so...>]

Bugs item #1505811, was opened at 2006-06-14 09:07
Message generated for change (Comment added) made by maartenbrock
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=100599&aid=1505811&group_id=599

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: C-Front End
>Group: fixed
>Status: Closed
>Resolution: Fixed
Priority: 5
Private: No
Submitted By: Goofy (gaufillet)
>Assigned to: Maarten Brock (maartenbrock)
Summary: Bad 'for' loop optimization

Initial Comment:
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.


----------------------------------------------------------------------

>Comment By: Maarten Brock (maartenbrock)
Date: 2008-03-10 22:40

Message:
Logged In: YES 
user_id=888171
Originator: NO

Fixed in SDCC #5087.

----------------------------------------------------------------------

Comment By: rlar (rlar)
Date: 2007-07-26 20:39

Message:
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;


----------------------------------------------------------------------

Comment By: rlar (rlar)
Date: 2007-07-22 21:59

Message:
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

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=100599&aid=1505811&group_id=599