Menu
▾
▴
#2600 Bug in how SDCCpeeph.c matches source lines and peephole rules
In function matchLine() at line 1905 of SDCCpeeph.c there is minor error in how a line of source text is matched against a line of a peephole rule.
This portion of code:
/* they should be an exact match other wise */
if (*s && *d)
{
if (*s++ != *d++)
return FALSE;
}
}
should only be running if the "destination is not a var". The previous "if" statement is taking care of the case of a "%" in the peephole rule. After taking care of that it exits the "if" and requires an exact match of the following character in the rule and source. This prevents rules having two successive % from being possible, for example:
replace restart {
ld c,l
ld b,h
%1 %2,%3
} by {
%1 %2,%3
ld c,l
ld b,h
; peephole z88dk-485c1
} if same(%1 'set' 'res' 'bit'), operandsNotRelated(%3 'bc')
In the above rule for z80 code, I am looking for any set,res,bit instruction so "%1 %2" appears in sequence. This rule is not currently matched by SDCCpeeph.c.
The fix is to make the exact match step part of the "else" part of the "if":
.....
while (ISCHARSPACE (*s))
s++;
while (ISCHARSPACE (*d))
d++;
}
else if (*s && *d)
{
/* they should be an exact match otherwise */
if (*s++ != *d++)
return FALSE;
}
}
/* get rid of the trailing spaces
in both source & destination */
.....
Fixed in revision [r9888].
Philipp
Philip, the patch applied is flawed. I found recently compiled programs were 2k+ larger because some peephole rules were not being applied.
The code that is in there now in function matchLine():
static bool matchLine (char *s, const char *d, hTab ** vars) { if (!s || !(*s)) return FALSE; /* skip leading white spaces */ while (ISCHARSPACE (*s)) s++; while (ISCHARSPACE (*d)) d++; while (*s && *d) { /* if the destination is a var */ if (*d == '%' && ISCHARDIGIT (*(d + 1)) && vars) { const char *v = hTabItemWithKey (*vars, keyForVar (d + 1)); /* if the variable is already bound then it MUST match with dest */ if (v) { while (*v) if (*v++ != *s++) return FALSE; } else /* variable not bound we need to bind it */ bindVar (keyForVar (d + 1), &s, vars); /* in either case go past the variable */ d++; while (ISCHARDIGIT (*d)) d++; } else if (ISCHARSPACE (*s) && ISCHARSPACE (*d)) /* whitespace sequences match any whitespace sequences */ { while (ISCHARSPACE (*s)) s++; while (ISCHARSPACE (*d)) d++; } else if (*s && *d) /* they should be an exact match otherwise */ { if (*s++ != *d++) return FALSE; } } /* skip trailing whitespaces */ if (*s) while (ISCHARSPACE (*s)) s++; if (*d) while (ISCHARSPACE (*d)) d++; /* after all this if only one of them has something left over then no match */ if (*s || *d) return FALSE; return TRUE; }is not functionally equivalent to this, which is working for me and is based on your code and code previous to the patch:
static bool matchLine (char *s, const char *d, hTab ** vars) { if (!s || !(*s)) return FALSE; while (*s && *d) { /* skip white space in both */ while (ISCHARSPACE(*s)) s++; while (ISCHARSPACE(*d)) d++; /* if the destination is a var */ if (*d == '%' && ISCHARDIGIT (*(d + 1)) && vars) { const char *v = hTabItemWithKey (*vars, keyForVar (d + 1)); /* if the variable is already bound then it MUST match with dest */ if (v) { while (*v) if (*v++ != *s++) return FALSE; } else /* variable not bound we need to bind it */ bindVar (keyForVar (d + 1), &s, vars); /* in either case go past the variable */ d++; while (ISCHARDIGIT (*d)) d++; } else if (*s && *d) { /* they should be an exact match otherwise */ if (*s++ != *d++) return FALSE; } } /* skip trailing whitespaces */ if (*s) while (ISCHARSPACE (*s)) s++; if (*d) while (ISCHARSPACE (*d)) d++; /* after all this if only one of them has something left over then no match */ if (*s || *d) return FALSE; return TRUE; }I didn't see any regression in the peephole rules that come with SDCC, Can you give an example peephole, where this bug is visible?
Philipp
It was brought to my attention in this project: https://bitbucket.org/CmGonzalez/pietro_bros/overview
The binary size increased from ~32700 bytes to ~34000 bytes. I could see that most peephole rules were not being applied.
The enemy_coin1 function can be used as example. This is code generated by 9880:
By 9889:
With 1200+ bytes increase in size there are a lot of instances like this.
I'll see if I can isolate this function and applicable rules.
--fverbose-asm might be helpful (if the rules have comments, like the ones that come with SDCC do).
Philipp
Yes that's how I found the rules.
I think it's only going to let me attach one file. So here is test.c:
#define GAME_LIN_FLOOR 152 extern unsigned char phase_left; extern unsigned char lin[8]; extern unsigned char col[8]; extern unsigned char sprite; extern void enemy_walk(void); extern void spr_destroy(unsigned char f_sprite) __z88dk_fastcall; void enemy_coin1(void){ //COINS N SLIPICE if ( phase_left > 0 ) { enemy_walk(); if ( ( lin[sprite] == GAME_LIN_FLOOR ) && ( col[sprite] == 3 || col[sprite] == 27) ) { spr_destroy(sprite); } } }and peep.def is attached.
To reproduce:
sdcc -v
9897 MINGW
First without the additional rules:
sdcc -mz80 -S --reserve-regs-iy test.c
It must be a reserve-regs-iy compile.
Then with the rules:
sdcc -mz80 -S --reserve-regs-iy test.c --peep-file peep.def
I can see that this only changes the last store in the translated function. I'm trying to find an earler sdcc now to verify more rules are applied.
Yes, sdcc #9883 is still available at the sdcc snapshot page and I've just verified that it does apply the full set of rules.
Edit: Or rather more of them. The output still doesn't match the 9880 at the original website. I'll check if I missed something in the peep.def file.
Edit: No nothing is missing. I think the discrepancy is down to there not being any interplay between the sdcc rules and peep.def in the examples we have here. In z88dk the two rulesets are together in the same file so they both modify at the same time. You may be able to get the same result by appending these rules to the existing sdcc ones before the jp/jr rules.
Last edit: alvin 2017-05-03
I see the difference. Previously, sequences of white space could match against nothing. Now sequences of white-space only match against other sequences of white-space.
This was actually an intentional change. After all, (on a hypothetic architecure with such instructions), we wouldn't want "add c b" to match "addc b".
But the change breaks your peephole rules that do not have whitespace after the comma.
I guess making ',' a special case, so that ',' matches ',' followed by whitespace should be an ok fix.
Philipp
The change in handling of whitespace after comma was implemented in [r9898].
Philipp