Sprite16 behavior? - Page 1

Hi,

Did something change from TIGCC in GCC4TI's Sprite8/16/32 routines? I downloaded and built the sources for the GCC4TI 0.96 Beta 10 release on my Ubuntu 8.04 system and discovered that the Sprite16 routine seems to act strange. It seems to be always putting sprites to the screen in some arbitrary mode regardless of the flag (SPRT_OR, SPRT_XOR, etc.) I pass to it.

On the other hand, the exact same project, when compiled with TIGCC 0.96 Beta 8 r1, operates perfectly. (Although I had to replace the estack.h header that it came with with GCC4TI's version to make it build, because otherwise I get an “Expected ‘)’ before ‘unsigned’” error on line 49 of estack.h, even when I try to build a source file that contains nothing but the lines “#include <estack.h>” and “void _main(void) { }”. This is the reason I gave GCC4TI a try in the first place.)

Let me know if more information is needed. It's been a few years since I last touched this stuff, so I'm probably forgetting things.

I've seen that bug myself, and I fixed it in GCC4TI SVN more than one month ago (r1360), but thanks for the report nonetheless
All routines successfully went through Joey Adam's exhaustive test program, i.e. this particular problem is not triggered by the program, because the re-declared Sprite8/16/32 functions' prototypes are different in that testcase...

To fix it, download a recompiled tigcc.a from http://www.mirari.fr/sncx (yAronet's upload area) and replace the prototypes of Sprite*, in $TIGCC/include/c/sprites.h, by:

extern void Sprite8(short asm("d0"),short asm("d1"),short asm("d2"),const unsigned char* asm("a0"),void* asm("a1"),short asm("d3"))__ATTR_LIB_ASM__;
extern void Sprite16(short asm("d0"),short asm("d1"),short asm("d2"),__cpushort asm("a0"),void* asm("a1"),short asm("d3"))__ATTR_LIB_ASM__;
extern void Sprite32(short asm("d0"),short asm("d1"),short asm("d2"),__cpulong asm("a0"),void* asm("a1"),short asm("d3"))__ATTR_LIB_ASM__;

Side notes:
* __attribute__((__regparm__(n))) (which __ATTR_LIB_C__ is) was (almost) banned from ExtGraph a while ago, so as to avoid this exact same problem. To play on the safe side, ASM routines need explicit register parameters.

* besides the Sprite8/16/32 fix, the recompiled tigcc.a contains 1) a code correctness fix + size optimization for bsearch() (which has been broken since before the creation of the TIGCC CVS repository in 2004, probably since the routine was integrated in TIGCC), 2) a significant size & speed optimization on qsort(), 3) further optimization of the ClipSprite8/16/32 routines (same prototype as Sprite8/16/32) that Joey Adams contributed to TIGCC years ago and 4) support for MIN_AMS 301 and 310.
New prototypes for bsearch() and qsort():

extern void *bsearch(const void* asm("a0"),const void* asm("a1"),short asm("d0"),short asm("d1"),compare_t asm("a2")) __ATTR_LIB_ASM__;
extern void qsort(void* asm("a0"),short asm("d0"),short asm("d1"),compare_t asm("a2"))__ATTR_LIB_ASM__;

Perfect, that seems to work. Thanks a lot! I should be back in business now.

Good

See now why I don't merge this kind of rewrites without careful reviewing and testing (which I didn't have the time for so far for Joey's routines)? (In addition, Joey also asked me not to merge those routines without adding the additional routines he did, which only recently got documentation in the correct format.)

And __ATTR_LIB_C__ has always been banned for assembly routines in TIGCCLIB, that's why it's called __ATTR_LIB_C__! Assembly routines are indeed supposed to use explicit register parameters. This has been the rule ever since we started using register parameters in TIGCCLIB.

Lionel Debroux (./3) :
1) a code correctness fix + size optimization for bsearch() (which has been broken since before the creation of the TIGCC CVS repository in 2004, probably since the routine was integrated in TIGCC),2) a significant size & speed optimization on qsort()

So you converted the C code to assembly and micro-optimized a few instructions in it. I'm not sure I'll be merging that, as it reduces maintainability significantly for very little gain.

In addition, why did you put the assembly code in an asm block in a .c file? Assembly functions should be in .s files!

And finally, I cannot merge your bsearch.c because it uses code under the BSD license which is not compatible with the TIGCCLIB license. (The BSD license requires documentation of binary-only programs to carry the copyright notice for the used code, which is a requirement not in the TIGCCLIB license's GPL exception.)

See now why I don't merge this kind of rewrites without careful reviewing and testing

(emphasis mine)
The routines themselves went through Joey's extra-strong test suite, but I forgot that Joey's testcase contains its own C protoypes for the routines, and the code just happened to work with the compilation options of the TPR. Add -mregparm and the testcase starts failing.

You're making the case of what we have stated for several months: TIGCC's test suite is dauntingly and appallingly incomplete. Fact.
You haven't run it for years, otherwise you'd have found and fixed no less than four or five problems. Fact.
You haven't backported most of our fixes to TIGCC, months after we made them. Fact.
We've started increasing the test coverage in GCC4TI. Fact. But it will take a lot of effort to raise GCC4TI up from the low-quality code base inherited from TIGCC.

(which I didn't have the time for so far for Joey's routines)?

You didn't _spend_ the time for Joey's routines so far. That's very different

So you converted the C code to assembly and micro-optimized a few instructions in it.

for very little gain.

These "micro-optimisations" represent a quarter of the size of both bsearch and qsort, and besides the size optimization, they're also speed optimizations (as demonstrated by the qsort test program I added to the repository).
Sprite* routines are both smaller and faster, while supporting 1 additional drawing mode, than the current TIGCC ones. The TIGCC ones have remained the same since the beginning, despite the first optimizations to the C version of the Sprite* routines being contributed by myself in 2002, further optimizations to the C version being contributed by myself in 2003, and Joey's contributions of 2005.

as it reduces maintainability significantly

Well, maybe, though I left the original C code on purpose. However, note that we added a test program for qsort, thereby filling one of the many many blanks of the TIGCC test suite

In addition, why did you put the assembly code in an asm block in a .c file? Assembly functions should be in .s files!

Judging by the way you complained when rebuilding the TIGCC IDE by Delphi 7 changed a few bitmaps, I thought you hated diff pollution by non-functional changes ? This is exactly what the change you're suggesting would yield in the TPR, and I fail to see a good reason for that change: assembly blocks in C files work just fine, they have worked just fine for years, and it would be a bug if they stopped working.

And finally, I cannot merge your bsearch.c because it uses code under the BSD license which is not compatible with the TIGCCLIB license. (The BSD license requires documentation of binary-only programs to carry the copyright notice for the used code, which is a requirement not in the TIGCCLIB license's GPL exception.)

The assembly code in bsearch is a hand optimization of the compiler output of some BSD-licensed code, of which I retained only the algorithm (which happens to be better than that used by Zeljko).
Even if you don't want to merge our bsearch.c as is for whatever reason, you still had better fix, sooner than later, the bsearch.c in TIGCC (which has been broken since before the TIGCC CVS repository was created), and credit PpHd for finding the bug while doing the job that yourself didn't do: testing the routine (for PedroM)

Kevin Kofler (./6) :
See now why I don't merge this kind of rewrites without careful reviewing and testing

Tu rigoles ou quoi, c'est ta méthode de release.

Lionel Debroux (./8) :
The routines themselves went through Joey's extra-strong test suite, but I forgot that Joey's testcase contains its own C protoypes for the routines, and the code just happened to work with the compilation options of the TPR. Add -mregparm and the testcase starts failing.

That proves that your testing was inadequate. And it's also why I don't trust automated testsuites, the only reliable way to test is write a trivial program inside KTIGCC which includes <tigcclib.h> and calls the function, build it and run it. And if the test passed, I don't need the testcase anymore, so I delete it.

You're making the case of what we have stated for several months: TIGCC's test suite is dauntingly and appallingly incomplete. Fact.

No, the fact is that TIGCC doesn't have a testsuite (what you call a "testsuite" is just a set of examples documenting some function, it's designed to be documentation, not a testsuite, and the examples are selected to illustrate certain points to a human reader, not to test TIGCCLIB) and there are no plans to add one.

But it will take a lot of effort to raise GCC4TI up from the low-quality code base inherited from TIGCC.

So far, you've added several bad regressions in frequently-used code due to poorly tested (note that I didn't write "untested", as you obviously tried to test the stuff, but failed) changes and fixed mostly bugs nobody actually encountered in practical use (or they wouldn't have stayed unnoticed for years). Bugginess can't be measured by a bug count alone, you also have to take the impact of the bugs into account.

You didn't _spend_ the time for Joey's routines so far. That's very different

I can't spend time I don't have.

This is exactly what the change you're suggesting would yield in the TPR, and I fail to see a good reason for that change: assembly blocks in C files work just fine, they have worked just fine for years, and it would be a bug if they stopped working.

The TIGCCLIB coding convention is that assembly functions go into assembly files (i.e. .s files, as A68k code is not allowed in TIGCCLIB), not C files. Just looking at the existing functions would have told you that. I want to know from a directory listing or from the file tree of the tigcc.tpr project in KTIGCC which functions are in C and which are in assembly.

Even if you don't want to merge our bsearch.c as is for whatever reason, you still had better fix, sooner than later, the bsearch.c in TIGCC (which has been broken since before the TIGCC CVS repository was created), and credit PpHd for finding the bug while doing the job that yourself didn't do: testing
the routine (for PedroM)

I agree the bug needs to be fixed, but replacing the whole function is a poor way of fixing bugs.

And it's also why I don't trust automated testsuites, the only reliable way to test is write a trivial program inside KTIGCC which includes <tigcclib.h> and calls the function, build it and run it.

I assume "build it" is a multiple step process, for testing multiple compilation option sets ? If not, the way you're describing is too simple-minded to catch calling convention mismatches (and possibly other errors, too)
Remember, this is the issue here, which also bit me on one or two TICT programs when switching from __stkparm__ mode to __regparm__ mode.

And if the test passed, I don't need the testcase anymore, so I delete it.

Well, that's your way of doing things. Other people may want to do differently

No, the fact is that TIGCC doesn't have a testsuite (what you call a "testsuite" is just a set of examples documenting some function, it's designed to be documentation, not a testsuite, and the examples are selected to illustrate certain points to a human reader, not to test TIGCCLIB) and there are no plans to add one.

I have already understood your point that the examples are meant as documentation, but you still haven't understood that the examples can be used to test TIGCCLIB, even without any code modifications (though modifications might be desirable on some examples if they're really too simple-minded), by varying startup code options.

But it will take a lot of effort to raise GCC4TI up from the low-quality code base inherited from TIGCC.

So far, you've added several bad regressions in frequently-used code due to poorly tested (note that I didn't write "untested", as you obviously tried to test the stuff, but failed) changes

Mistakes can, and do, happen, when doing something productive such as optimizing and adding features. Those who seldom do anything productive have little risk to make mistakes.

You didn't _spend_ the time for Joey's routines so far. That's very different

I can't spend time I don't have.

You spend lots of time trolling and finding excuses for not doing your maintainership work, so you do have time. You choose to spend it on improductive things... it's your call, but such statements as "I can't spend time I don't have" are plain lies, and repeating them doesn't make them true, sorry.

I want to know from a directory listing or from the file tree of the tigcc.tpr project in KTIGCC which functions are in C and which are in assembly.

I don't see the urge for that, but it can be argued to be a valid reason.

I agree the bug needs to be fixed, but replacing the whole function is a poor way of fixing bugs.

We didn't plan to replace the whole code of bsearch and qsort just for the sake of it, you know
* the code of the buggy bsearch() was simply compared with other, known-correct implementations. A better algorithm was found among those. Later, the assembly generated by the compiler was hand-edited because GCC did a crappy job.
* qsort() was reported to be slow, and a quick examination of the shellsort algorithm used in qsort() showed that it used Shell's original algorithm, which exhibits O(n^2) behaviour. It has been known for years (and documented on WP) that very minor tweaks to the algorithm make its worst case behaviour much better. So we went for tweaking the algorithm, using a better (= smaller AND faster !) algorithm. And since the compiler was doing a crappy job, I resorted to tweaking the assembly code, yielding a savings of 34 bytes in 7 steps.

Lionel Debroux (./11) :
I assume "build it" is a multiple step process, for testing multiple compilation option sets ? If not, the way you're describing is too simple-minded to catch calling convention mismatches (and possibly other errors, too)

If you test a header whose prototypes are autogenerated by the HSF tools, the calling convention will necessarily be independent of the -mregparm switch used, our tool automatically adds an attribute for that. Your prototypes were wrong no matter what -mregparm switch was used on the command line, as __ATTR_LIB_C__ sets a fixed number of register parameters, overriding the switch on the command line.

You spend lots of time trolling and finding excuses for not doing your maintainership work, so you do have time.

I'd need both a lot more time and a lot more concentration to do that work than to discuss things on message boards. Usually, when I frequent the forums, it's when I'm too tired to do anything else.