From: SourceForge.net <no...@so...> - 2004-02-02 17:21:42
|
Feature Requests item #877103, was opened at 2004-01-14 23:42 Message generated for change (Comment added) made by stsp You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=350599&aid=877103&group_id=599 Category: None Group: None >Status: Closed Priority: 5 Submitted By: Stas Sergeev (stsp) Assigned to: Bernhard Held (bernhardheld) Summary: regress: signedness confusion and code increase Initial Comment: Hi. There seem to be a bug added recently (during the New Year hollidays actually), which causes sdcc to confuse signed vs unsigned operands, which leads to a huge code size increase. Here is the example which, being compilled with the sdcc from CVS HEAD, is 3 times larger than compilled with the month old sdcc. --- #include <at89S8252.h> struct ttt { char a; short b; }; char main() { xdata struct ttt arr[5]; unsigned char idx, dv; idx = SBUF + 3; dv = SBUF + 4; idx %= dv; dv = SBUF + 1; idx /= dv; return arr[idx].a; } --- Code size with current sdcc: 475b Code size with a month old sdcc is about 170b. ---------------------------------------------------------------------- >Comment By: Stas Sergeev (stsp) Date: 2004-02-02 20:21 Message: Logged In: YES user_id=501371 Excellent work, many thanks! ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-02-01 01:05 Message: Logged In: YES user_id=203539 All done. HTH! ---------------------------------------------------------------------- Comment By: Stas Sergeev (stsp) Date: 2004-01-29 20:55 Message: Logged In: YES user_id=501371 Hi again. What do you think about an optimisation of an access to the small arrays that can be indexed by an "unsigned char"? I had a patch for that, it is touching only the AST code. Since you said nothing should be done on icode level, I gather perhaps the patch is even correct. It saves 38 bytes for me, making the regression almost invisible. The patch is attached, please review it if you have some time. As a bonus, it adds the warning about violating the array boundaries. But its optimization part works only with SDCC_NEWTYPEFLOW (which is expected after all). I am setting that RFE to "Open" only to make sure that this pending stuff I posted here will not be forgotten. ---------------------------------------------------------------------- Comment By: Stas Sergeev (stsp) Date: 2004-01-28 20:41 Message: Logged In: YES user_id=501371 After playing with that patch for the whole day, I think I have a positive experience with it after all. Initially the program grown by ~700 bytes again, but the studying of a generated asm revealed that this time sdcc uses ints where it actually makes sense, and the explicit cast to "unsigned char" is quite a cure. But after intersecting the whole source with the explicit casts, I still can't get back something about ~70 bytes. In principle this is tolerable, but I want you to know that there are still a misses somewhere, and sdcc still uses ints for no reason sometimes. The example demonstrates it: --- #include <at89S8252.h> unsigned char func(unsigned char arg) { return arg + 1; } char main() { unsigned char a; a = SBUF + 1; { unsigned char b = a << 1; return func(1 << b); } } --- Compiling without SDCC_NEWTYPEFLOW: 159 bytes With SDCC_NEWTYPEFLOW: 173 bytes (regress!) If you can fix also this, would be really nice:) There will be no regressions (for me) at all then. And putting the regressions aside, I realize how important your fixes were. In fact I have discovered a few cases where I actually needed int and it was not there, but now it is. Which of course saved me several hours of a debugging work:) ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-01-27 13:00 Message: Logged In: YES user_id=203539 Ahh, I see. Fixed. With the new AST-code we've new possibilities and don't need to (badly) fix things in icode! Thanks for your continued patience. It's sometimes hard for me to keep the overview over ast, icode and assembler output. Then some input from somebody else is very helpfull. ---------------------------------------------------------------------- Comment By: Stas Sergeev (stsp) Date: 2004-01-27 07:40 Message: Logged In: YES user_id=501371 OK, I think any example that does the left-shifting will be the case. Here is a trivial one: --- #include <at89S8252.h> unsigned char main() { unsigned char c, p; p = SBUF + 3; c = 1 << p; return c; } --- Compiling without patch: 153 bytes. With patch: 144 bytes. SDCC_NEWTYPEFLOW doesn't seem to affect the size for that example at all. Everything is "unsigned char" in that example. Attached. ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-01-27 00:01 Message: Logged In: YES user_id=203539 The mailling list have a problem today, I didn't even receive my own mail from yesterday :-( And yes, the patch should be obsolete. I don't understand, why you still need it. If you could provide a code snippet I'll have a look at your problem. ---------------------------------------------------------------------- Comment By: Stas Sergeev (stsp) Date: 2004-01-26 22:35 Message: Logged In: YES user_id=501371 Thanks, now that really helps! With that fix I was finally able to use a new sdcc for my program and even got a smaller code than with a month old sdcc. But this doesn't replace the patch you supplied in a #832664 which gives an old behaveour for shifting. Now that patch is rejecting and I had to manually adjust it - it still helps. Something in the code hints me that SDCC_NEWTYPEFLOW was supposed to fix also that one (otherwise why would it touch the same line the aforementioned patch touches), but it doesn't. If it wasn't actually supposed that way - just bypass that. Just trying to make sure that nothing was forgotten. Sorry, I was not able to find a discussion. Perhaps I am blind, will try again later. I was looking here: https://sourceforge.net/mailarchive/forum.php?forum_id=4107&max_rows=25&style=ultimate&viewmonth=200401 ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-01-26 00:36 Message: Logged In: YES user_id=203539 Fixed, see ChangeLog 1.605 The new type flow is enabled with the environment variable SDCC_NEWTYPEFLOW Discussion in sdcc-user ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-01-20 00:31 Message: Logged In: YES user_id=203539 Good catch! The patch is applied in SDCCicode.c 1.183 ---------------------------------------------------------------------- Comment By: Stas Sergeev (stsp) Date: 2004-01-19 20:52 Message: Logged In: YES user_id=501371 OK, your patch didn't help:) Well, since it fixes the test-case, I can only blame myself for introducing the test-case that doesn't reflect the real problem. Unfortunately for some reasons my program was unaffected by your patch. Anyway, I was using another patch for quite some time, which helps a bit more. I thought it was only a hack since it helped only for the array access, but not for the division/multiplication. But as you say those are the different problems, I figured the patch may actually be correct. It is attached. Please see if it is OK to apply. It reduces the code for array access by making the size unsigned. Unfortunately it only returns something about ~70 bytes, which is still too far from being sufficient. ---------------------------------------------------------------------- Comment By: Stas Sergeev (stsp) Date: 2004-01-19 08:03 Message: Logged In: YES user_id=501371 Thanks for the fix! I'll see how many bytes will it return to me. > But they return correct results now, therefore this > is just a "feature request" for me. Yes, I know they are correct, but as there is a regression (much increased code size) involved, I attributed (probably wrongly) it to bugs. > And I don't see any "confusion" about signedness here. The confusion was that the array size was signed, which imho makes no sense (and not any more). ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-01-19 00:57 Message: Logged In: YES user_id=203539 The offset calculation in arr[idx].a is fixed in SDCCicode.c 1.182 ('mul a,b' instead of _mulint()). This is the only real bug I can seen. The div and mod operation use integers. This is a known issue, and it's a regression. But they return correct results now, therefore this is just a "feature request" for me. And I don't see any "confusion" about signedness here. The promotion from "unsigned char" to "signed int" is perfectly in conformance with the C standard. This has already been discussed in bug #832664. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=350599&aid=877103&group_id=599 |