SDCC developers are working on a SDCC 3.8.0 release. There will probably be a RC1 next week followed by a release the week after. If any serious issues are found in SDCC, the release might be delayed.
I guess now and next week would be a good time for z88dk developers to test current SDCC trunk to see if there are any serious regressions or other bugs that affect z88dk.
Philipp
There is now a release candidate 1; unless issues that important enough to delay the release are found, the final 3.8.0 release will be in 10 days.
Philipp
I still had subtle bugs in the last update I tried but I think they may be in our peephole rules. I think the way to move forward is to get sdcc's regression tests running with our version of zsdcc to help locate where these problems may be.
@aralbrec
Issue #817 is not related to peephole rules: it is fixed by --max-alloc but it is present with no optimization. It may be related to some ZX81 routines, though.
Issue #819 is only occuring if sdcc is used with --opt-code-size. Only some targets seem impacted though (vz200, svi and maybe others).
I found a bug in sdcc z80 peephole rule 10:
replace restart {
ld %1, %2
ld %3, %4
ld %5, %1
} by {
ld %5, %2
; peephole 10 loaded %5 from %2 directly instead of going through %1.
ld %3, %4
} if canAssign(%5 %2), notVolatile(%1), operandsNotRelated(%1 %4), operandsNotRelated(%1 %3), operandsNotRelated(%4 %5), notUsed(%1), notSame(%3 %4 '(hl)' '(de)' '(bc)'), notVolatile(%5)
This is being erroneously applied to this code fragment:
ld a,(hl)
ld hl,(#_foo)
ld h,a
with:
%1 = a
%2 = (hl)
%3 = hl
%4 = (#_foo)
%5 = h
I'm testing a replacement with different condition:
if notUsed(%1), canAssign(%5 %2), notVolatile(%1), notVolatile(%5), operandsNotRelated(%1 %3), operandsNotRelated(%1 %4), operandsNotRelated(%2 %3), operandsNotRelated(%3 %5), operandsNotRelated(%4 %5), notSame(%3 %4 '(hl)' '(de)' '(bc)')
That looks like a serious issue, maybe serious enough to delay the release. operandsNotRelated(%3 %5) should be sufficient to fix it.
Do you have a small, compile able C code sample to reproduce the issue (from which I could make a regression test)?
Philipp
The code fragment is this:
typedef unsigned char type8;
extern void set_info(type8 b) __z88dk_fastcall;
extern void set_arg1(void);
extern void set_arg2_nosize(type8 use_dx, type8 b);
extern void do_bop(type8 b, type8 a);
extern type8 byte1;
extern type8 byte2;
extern type8 *arg2;
void check_btst(void)
{
set_info((type8) (byte2 & 0x3f));
set_arg1();
set_arg2(1, byte1);
do_bop(byte2, arg2[0]);
}
The peephole rule is eliminating the second byte parameter load arg2[0] in the final do
_bop() call.
5603 1A5A _check_btst:
5604 1A5A 3A 6D 00 ld a,(_byte2)
5605 1A5D E6 3F and a,0x3f
5606 1A5F 6F ld l, a
5607 1A60 CD 30 0B call _set_info
5608 1A63 CD 75 0B call _set_arg1
5609 1A66 3A 6C 00 ld a,(_byte1)
5610 1A69 57 ld d,a
5611 1A6A 1E 01 ld e,0x01
5612 1A6C D5 push de
5613 1A6D CD E0 0F call _set_arg2
5614 1A70 F1 pop af
5615 1A71 2A 6D 00 ld hl, (_byte2) ;;; arg2[0] is gone
5616 1A74 E5 push hl
5617 1A75 CD 09 1A call _do_bop
5618 1A78 F1 pop af
5619 1A79 C9 ret
This can't be duplicated in sdcc alone as there is interaction with other z88dk rules that changes the code but sdcc peephole 10 does the erroneous transformation.
With peephole 10 eliminated (do_bop call only):
19728 39CD ; genAssign
19729 39CD ;fetchPairLong
19730 39CD ;fetchLitPair
19731 39CD ;fetchLitPair
19732 39CD ;fetchLitPair
19733 39CD ; peephole z88dk-216c
19734 39CD ; peephole z88dk-171
19735 39CD ; genPointerGet
19736 39CD 2A 00 00 ld hl,(_arg2)
19737 39D0 7E ld a,(hl)
19738 39D1 ; peephole z88dk-96b
19739 39D1 ; peephole z88dk-339a
19740 39D1 ; peephole z88dk-366
19741 39D1 ; peephole z88dk-339a
19742 39D1 ; genIpush
19743 39D1 ; peephole 1 removed dead load from a into b.
19744 39D1 ; peephole z88dk-391a
19745 39D1 ; genIpush
19746 39D1 ;fetchLitPair
19747 39D1 ;fetchLitPair
19748 39D1 ; peephole 17 loaded a from (#_byte2) directly instead of using hl.
19749 39D1 2A 00 00 ld hl,(_byte2)
19750 39D4 67 ld h,a
19751 39D5 ; peephole 1 removed dead load from l into a.
19752 39D5 E5 push hl
19753 39D6 ; peephole z88dk-481q1
19754 39D6 ; genCall
19755 39D6 CD 65 39 call _do_bop
19756 39D9 F1 pop af
19757 39DA ; genLabel
19758 39DA ; peephole 149 removed unused label l_check_btst_00101$.
19759 39DA ; genEndFunction
19760 39DA C9 ret
Peephole 10 transforms:
19737 39D0 7E ld a,(hl)
19749 39D1 2A 00 00 ld hl,(_byte2)
19750 39D4 67 ld h,a
to
ld h,(hl)
ld hl,(_byte2)
which is incorrect.
So I think it would be unusual for sdcc-generated code to run into this bug but it is a bug. Looking at the file this comes from as a whole, peephole 10 is probably applied (correctly) ten times in other places.
The fix won't make it into 3.8.0 (but is in the "next" branch, to be merged immediately after the release).
See also https://sourceforge.net/p/sdcc/bugs/2814/
Philipp
As a reference point z88dk-libraries FATFS library, compiled on August 31 had an error which has been apparent since my beginning with this library on Z80 machines. Compiling with the same zsdcc #9958 binary and same command line, the library now does not exhibit the same error. I think this S03 peephole bug fix has addressed a long standing issue. :+1:
Background: reading and listing ls in the root directory of a FATFS drive worked, but listing a sub-directory would result in a random number of files with random file names. That behaviour has now ceased, and correct sub-directory listings are obtained.
Is that the only problem with fatfs you have noticed? I am thinking about splitting it up into functions and incorporating it into the library.
Yes. I've found the ChaN FatFS libraries very stable, and usually without issues on the AVR (gcc) platform. Generally, I've not changed them at all from the original source.
With the SDCC I was having the issue that seemed to be resolved by the above fix. Beyond a recompilation with the new z80asm object version and the patch above, there were no changes that I'm aware of.
My only remaining issue with YAZ180 is that I've not allocated enough heap to permit changing directories, so the cd function returns with "Not enough core". But, copying files into and out of sub-directories now works too. Edit: stupid error on my side.
When the libraries were first established, we made some small MACROs to optimise on little ended Z80, and I've adjusted which functions get built for the RC2014 in read only mode, but that is all.
3.8.0 was released a while ago.
Today, upstream SDCC has enabled regression testing for the z80 backend with --reserve-regs-iy, all tests pass. Essentially, at this point, the code generated with --reserve-regs-iy should be as bug-free as the one wtihout.
Philipp
Assuming that SDCC is now correct, I've prepared a patch file for Version 3.8.2 #10649
I've tested it by compiling ChaN FatFS, and I am using it for RC2014 CP/M-IDE and YAZ180 yabios.
Most of the changes are to support the z88dk specifics, but there are one or two issues to be resolved in sdcc.
Z80 in argument search needs to be offset by 3 bytes (not 4 bytes).
if (!IS_GB && !IS_RAB && ISINST(pl->line, "in"))
return(!strstr(strchr(pl->line + 3, ','), "(c)") && (*what == 'a') || strstr(strchr(pl->line + 3, ','), "(c)") && ((*what == 'b') || (*what == 'c')));
Z180 out0 argument search needs to be offset by 5 bytes (not 4 bytes).
if(IS_Z180)
{
// snip
if (ISINST(pl->line, "out0"))
return(argContPrec(pl->line + 5, what, 2));
}
Changing the name of _memcpy to ___memcpy breaks when sdcc libraries are not used.
/* Now we can be sure to have found a builtin function. */
if ((!strcmp (bif->name, "__builtin_memcpy") || !strcmp (bif->name, "__builtin_strncpy") || !strcmp (bif->name, "__builtin_memset")) &&
IS_OP_LITERAL (IC_LEFT (lastparam)) && !operandLitValue (IC_LEFT (lastparam)))
{
/* We have a builtin that does nothing. */
/* TODO: Eliminate it, convert any SEND of volatile into DUMMY_READ_VOLATILE. */
/* For now just convert back to call to make sure any volatiles are read. */
strcpy(OP_SYMBOL (IC_LEFT (icc))->rname, !strcmp (bif->name, "__builtin_memcpy") ? "_memcpy" : (!strcmp (bif->name, "__builtin_strncpy") ? "_strncpy" : "_memset"));
goto convert;
}
if ((TARGET_IS_Z80 || TARGET_IS_Z180 || TARGET_IS_RABBIT) && (!strcmp (bif->name, "__builtin_memcpy") || !strcmp (bif->name, "__builtin_strncpy") || !strcmp (bif->name, "__builtin_memset")))
{
/* Replace iff return value is used or last parameter is not an integer constant. */
if (bitVectIsZero (OP_USES (IC_RESULT (icc))) && IS_OP_LITERAL (IC_LEFT (lastparam)))
return;
strcpy(OP_SYMBOL (IC_LEFT (icc))->rname, !strcmp (bif->name, "__builtin_memcpy") ? "_memcpy" : (!strcmp (bif->name, "__builtin_strncpy") ? "_strncpy" : "_memset"));
goto convert;
}
Since the patch file is pretty short, perhaps it can be further tested and reviewed, please?
I've done what I can.
I have a few questions on the patch:
1) There are a few changes that seem to be only cosmetic. How about removing them to keep the diff small (and thus the patch more maintainable)?
1a) Indentation change in src/SDCCmain.c on the line
~~~
fprintf (stream, "published under GNU General Public License (GPL)n");
~
1b) Moving of variable declaration for lic, ric, litval in src/SDCCopt.c
1c) Reordering of conditions in src/SDCCpeeph.c:
~~~
while ((*bp != 'n' && *bp != '}') && *bp)
~
1d) Adding of {, } around a 1 -line else-block in src/SDCCpeeph.c.
1e) TRUE vs. true in z80SurelyReturns() in src/SDCCpeeph.c.
2) SDCC intentionally uses __memcpy() for the implementation of memcpy(): When string.h is not included, the C standard allows the programmer to use a static (but not extern) identifier "memcpy". So when we want to insert a call to memcpy() for struct assignment, we can't just call whatever symbol the name memcpy resolves to: It might be a static variable or function unrelated to memcpy() from the standard library. How about making the change to __memcpy in z88dk libraries, too?
Philipp
Edit: Fixed two typos.
OK. I've done another version that changes just the minimum.
Although since z88dk doesn't use the sdcc peep.c the changes there could also be removed. I'd prefer to leave them in. As noted by @aralbrec in #1014 the order of examination of the opcodes was not completely correct, so I've reordered them from most specific (longest) to shortest.
Bump @spth are the edits problematic for the current sdcc release?
It would be good to know whether they cause any of your tests to fail.
@aralbrec the diff loses nothing from the current patch, but the question is whether the 3.8.2 release has added any regressions? I guess that only shows up (much) later.
3.8.2 is a development version, not a release. The latest release is 3.8.0. If you want a low risk of regressions, the 3.8.0 release would be a better choice.
Philipp
I've just tried the patch on current SDCC 3.8.3 #10679. I have not rebuild the libraries. In that setup, the regression test pass for z80 (both with and without --reserve-regs-iy), except for two that report a linker error. So in general, this looks quite good.
Philipp
@spth if the patch passes your tests (and is otherwise benign), then should I remove the 2 z88dk specifics below, and submit it into the SDCC bug tracker, with correct chunk offsets for #10679?
_memcpy to ___memcpy breaks when sdcc libraries are not usedWell, those two tests failing at link time should be looked into. And one should test also the effect on the libraries, and the other z80-related backends (z180, gbz9, r2k, r3ka, tlcs90). Testing is easy: Apply the patch to a current source tarball or svn. Build sdcc. Build the regression tests.
SDCC has a patch tracker separate from the bug tracker (https://sourceforge.net/p/sdcc/patches/). For submission there, it would probably be good to split the patch into thematic bits, instead of one big patch.
The two link tests are probably due to the ___memcpy change. They don't appear with the _memcpy patch removed.
I've divided the patch into three parts 1. for z88dk short calls, 2. for z88dk special functions, and 3. for rearranging the peephole optimiser.
sdcc-z88dk-feilipu-peephole-10704.patch
sdcc-z88dk-feilipu-shortcall-10704.patch
sdcc-z88dk-feilipu-specialfunctions-10704.patch
There is a failure of one regression test in bug-3495411.c. I think the failure is because an EXTERN is being handled differently. Not sure what the correct method should be.
This is the offending patch, I believe...
Index: src/SDCCglue.c
===================================================================
--- src/SDCCglue.c (revision 10704)
+++ src/SDCCglue.c (working copy)
@@ -365,11 +365,15 @@
emitDebugSym (&map->oBuf, sym);
dbuf_printf (&map->oBuf, "==.\n");
}
- if (IS_STATIC (sym->etype) || sym->level)
- dbuf_tprintf (&map->oBuf, "!slabeldef\n", sym->rname);
- else
- dbuf_tprintf (&map->oBuf, "!labeldef\n", sym->rname);
- dbuf_tprintf (&map->oBuf, "\t!ds\n", (unsigned int) size & 0xffff);
+ if (!IS_EXTERN (sym->etype))
+ {
+ if (IS_STATIC (sym->etype) || sym->level)
+ dbuf_tprintf (&map->oBuf, "!slabeldef\n", sym->rname);
+ else
+ dbuf_tprintf (&map->oBuf, "!labeldef\n", sym->rname);
+
+ dbuf_tprintf (&map->oBuf, "\t!ds\n", (unsigned int) size & 0xffff);
+ }
}
sym->ival = NULL;
what happens is that this piece of code from the unpatched version
;--------------------------------------------------------
; ram data
;--------------------------------------------------------
.area _INITIALIZED
_Sip_pCurLcb::
.ds 2
;--------------------------------------------------------
; absolute external ram data
;--------------------------------------------------------
looks like this after patching.
;--------------------------------------------------------
; ram data
;--------------------------------------------------------
.area _INITIALIZED
;--------------------------------------------------------
; absolute external ram data
;--------------------------------------------------------
It looks like z88dk handles EXTERN declarations differently, as this further patch creates a list.
Index: src/SDCCglue.c
===================================================================
--- src/SDCCglue.c (revision 10704)
+++ src/SDCCglue.c (working copy)
@@ -1827,9 +1831,12 @@
/* for all variables in this segment do */
for (sym = setFirstItem (map->syms); sym; sym = setNextItem (map->syms))
{
- /* if it is "extern" then do nothing */
+ /* if it is "extern" then add to the extern table */
if (IS_EXTERN (sym->etype) && !sym->ival)
- continue;
+ {
+ addSetHead (&externs, sym);
+ continue;
+ }
/* eliminate redundant __str_%d (generated in stringToSymbol(), SDCCast.c) */
if (!isinSet (tmpSet, sym))
Thoughts on how to resolve this?
I've applied the first part of sdcc-z88dk-feilipu-peephole-10704.patch (i.e. the one that adds two functions and handling of comments before the condition of a peephole. I'll have to look into the second part more before applying it.
Philipp
I've applied most of sdcc-z88dk-feilipu-shortcall-10704.patch. I excluded the {} in SDCC.y, which I don't understand, and the extern stuff that might cause a regression.
Philipp
I don't understand the z88dk EXTERN handling process, so I can't comment. I guess @aralbrec would be the best to know.
It may be that this piece of the patches remains outside SDCC because z88dk is simply different, and that is not a problem as I see it. The end goal is not to eliminate the differences, but simply to minimise them.
To avoid doubt, I assume you're going to look at the sdcc-z88dk-feilipu-specialfunctions-10704.patch code for z80/peep.c separately too? It wasn't mentioned, and the code is simple.
Thank you for reviewing the z80/peep.c part of the sdcc-z88dk-feilipu-peephole-10704.patch.
It is a fairly substantial reordering of the rules, and hasn't been reviewed. I did the reordering based on the issue that removal of the /t character from pattern matching requires the matches to be strictly in order longest opcode to shortest opcode. I'm not 100% certain I got it 100% correct.
I want to look at the z80-peephole parts of sdcc-z88dk-feilipu-peephole-10704.patch and sdcc-z88dk-feilipu-specialfunctions-10704.patch, but probably won't get around to doing it this week.
However, that list of functions in sdcc-z88dk-feilipu-specialfunctions-10704.patch seems too z88dk-specifc for SDCC at this point (I don't see a good use for them outside of zsdcc; on the other hand the short calls merged yesterday are at least useful for someone who wants to call z88dk functions from sdcc-compiled code).
Philipp
@spth Philipp, I think I've found an issue present in the r10748 release.
Comparison of the r9958 version against r10748 with the optimiser turned off shows a problem.
This is the fragment of C.
bank_get_abs((int8_t)atoi(args[1]))
This is the correct assembly found in r9958
2212 073A ;main.c:366: bankLockBase[ bank_get_abs((int8_t)atoi(args[1])) ] = 0x00;
2213 073A C5 push bc
2214 073B 6B ld l, e
2215 073C 62 ld h, d
2216 073D CD 00 00 call _atoi_fastcall
2217 0740 5D ld e, l
2218 0741 54 ld d, h
2219 0742 C1 pop bc
2220 0743 6B ld l, e
2221 0744 CD 00 00 call _bank_get_abs_fastcall
This is the incorrect assembly in r10748
2177 0730 ;main.c:366: bankLockBase[ bank_get_abs((int8_t)atoi(args[1])) ] = 0x00;
2178 0730 C5 push bc
2179 0731 6B ld l, e
2180 0732 62 ld h, d
2181 0733 CD 00 00 call _atoi_fastcall
2182 0736 5D ld e, l
2183 0737 54 ld d, h
2184 0738 C1 pop bc
2185 0739 1E 00 ld e,0x00
2186 073B 16 00 ld d,0x00
2187 073D 6B ld l, e
2188 073E 26 00 ld h,0x00
2189 0740 CD 00 00 call _bank_get_abs_fastcall
And another piece of bad assembly from r10748
3030 0C5F ;main.c:527: fprintf(output,"Saving Bank %01X to \"%s\"", bank_get_abs((int8_t)atoi(args[1])), args[2] );
3031 0C5F DD 6E EF ld l,(ix-17)
3032 0C62 DD 66 F0 ld h,(ix-16)
3033 0C65 4E ld c, (hl)
3034 0C66 23 inc hl
3035 0C67 46 ld b, (hl)
3036 0C68 DD 6E ED ld l,(ix-19)
3037 0C6B DD 66 EE ld h,(ix-18)
3038 0C6E 5E ld e, (hl)
3039 0C6F 23 inc hl
3040 0C70 66 ld h, (hl)
3041 0C71 C5 push bc
3042 0C72 6B ld l, e
3043 0C73 CD 00 00 call _atoi_fastcall
3044 0C76 5D ld e, l
3045 0C77 54 ld d, h
3046 0C78 C1 pop bc
3047 0C79 1E 00 ld e,0x00
3048 0C7B 16 00 ld d,0x00
3049 0C7D 6B ld l, e
3050 0C7E 26 00 ld h,0x00
3051 0C80 CD 00 00 call _bank_get_abs_fastcall
Note that this issue doesn't happen everywhere. Just where the result of the calls is an index to an array. The issue seems to be the insertion of the zero setting of e and d registers, without due consideration. The attached files show that it works everywhere else.
Attached two listing files showing the issue.
main_9958_SO0.c.lis
main_10748_SO0.c.lis
Just to confirm that this issue is apparent in the unpatched SDCC version, I compiled the same file with the r10748 repo version.
The problem fragment is this
bank_get_abs((int8_t)atoi(args[
It occurs 17 times in the main.c file, and quite a few of them have the atoi() return value stomped on by spurious ld e,0x00 and ld d,0x00 instructions.
main.c.asm
Out of interest, I removed the int8_t casting. This didn't fix the problem.
main_no_int8_t.c.asm
I've made a bug report #2854 in the SDCC repo too, which has been resolved as of r10754.
Ok. Now with r10757, I've made two patch files.
One for z88dk specifics, that should be all that is required to be the new master patch.
sdcc-10757-z88dk-specifics.patch
One for fixes to the sdcc z80 patch.c file. These should be reviewed by @spth, as it is a fairly substantial reordering of the rules, and hasn't been reviewed. I did the reordering based on the issue that removal of the /t character from pattern matching requires the matches to be strictly in order longest opcode to shortest opcode.
sdcc-10757-z88dk-peep.patch
Generally, imho, the r10757 code looks better than r9958. There has been a lot of improvement with register allocations, before peephole optimisation. Most of the improvement is invisible when using the z88dk specific peephole optimisation, but I guess it is better to have better original code and not have to rely on so much on peephole optimisation.
Are there internal z88dk sync reasons for waiting furtherly with https://github.com/z88dk/z88dk/pull/1024 ?
I've applied most of sdcc-z88dk-feilipu-shortcall-10704.patch. I excluded the {} in SDCC.y, which I don't understand, and the extern stuff that might cause a regression.
I've been busy so haven't been able to respond but will look at these things in the next couple of days.
(continuing from premature posting above)
This change in SDCC.y is for compatibility with more versions of bison. The SDCC.y file as-is is rejected by the version of bison (I think gnu) that I have installed on my windows machine.
Ok. I've done another update on the patch file to cover the issue with GNU Bison in SDCC.y
sdcc-10788-z88dk-specifics.patch.txt
Index: src/SDCC.y
===================================================================
--- src/SDCC.y (revision 10788)
+++ src/SDCC.y (working copy)
@@ -185,6 +185,10 @@
cleanUpLevel (SymbolTab, 1);
}
| addressmod
+ {
+ /* Reported by @aralbrec that GNU Bison requires this.
+ */
+ }
;
The attached patch file includes this fix, so if @spth updates SDCC.y, then we can delete it again.
Given the remaining patch is simply specifics for Z88DK, and all the peephole repair stuff has been submitted separately to SDCC patch tracker, would it make sense to try to get onto SDCC 3.8.x as default?
I'm still trying to find out why the {} is needed, as I never encountered a bison / yacc requiring it. It seems odd to only require it on this rule, but not others. Nevertheless, the workaround seems to be useful to some, so I'd apply it before 3.9.0 even if I don't really understand the issue by then: https://sourceforge.net/p/sdcc/bugs/2858/
Philipp
I'd recommend to base zsdcc by default on a release version of SDCC (i.e. x.y.0) as those tend to contain fewer bugs. SDCC 3.9.0 might be released relatively early in 2019. See https://sourceforge.net/p/sdcc/wiki/SDCC%203.9.0%20Release/ for the current state.
Philipp
Philipp
Probably then, a re-Title of this issue is appropriate.
:stuck_out_tongue_closed_eyes:
Added PR #1071 which will allow some more robust testing of r10892 and (hopefully no) issues to be reported, prior to finalising 3.9.0.
@spth with PR #1071 and #1074 the migration to sdcc 3.8.5 is complete and wrapped.
Let鈥檚 hope that bugs to 3.9.0 are benign and rare.
Noted sdcc bug #2875 that was resolved from r10960.
Subsequent IO port reads are fairly common, this one is a relevant fix.