[sdcc-devel] [ sdcc-Feature Requests-877103 ] regress: signedness confusion and code increase

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

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: Open
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-12 20:25

Message:
Logged In: YES 
user_id=501371

> I'm a little bit 
> supprised that you're already happy ;-)
Not all the hopes are lost yet:)
I've finally found a missing regression today.
Here it is:
---
#include <at89S8252.h>

unsigned char main() {
  unsigned char a;
  a = SBUF + 3;
  a %= 5/1;
  return a;
}
---
Compiled with old sdcc: 139 bytes
With latest one: 232 bytes

Signedness confusion again perhaps, although it goes
away when I change SDCCsymt.c:1578 to make the result
*signed*, while it should be the other way around,
at least it was with the previous ones...

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

Comment By: Stas Sergeev (stsp)
Date: 2004-02-12 20:24

Message:
Logged In: YES 
user_id=501371

> I'm a little bit 
> supprised that you're already happy ;-)
Not all the hopes are lost yet:)
I've finally found a missing regression today.
Here it is:
---
#include <at89S8252.h>

unsigned char main() {
  unsigned char a;
  a = SBUF + 3;
  a %= 5/1;
  return a;
}
---
Compiled with old sdcc: 139 bytes
With latest one: 232 bytes

Signedness confusion again perhaps, although it goes
away when I change SDCCsymt.c:1578 to make the result
*signed*, while it should be the other way around,
at least it was with the previous ones...

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

Comment By: Bernhard Held (bernhardheld)
Date: 2004-02-03 10:35

Message:
Logged In: YES 
user_id=203539

I thank you too for your hints and testing. I'm a little bit 
supprised that you're already happy ;-) There are still some 
chances to make SDCC better -- I'm currently working to 
avoid any promotion around 8 bit mul/div/mod operations.

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

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

[sdcc-devel] [ sdcc-Feature Requests-877103 ] regress: signedness confusion and code increase

The Small Device C Compiler (SDCC), targeting 8-bit architectures

[sdcc-devel] [ sdcc-Feature Requests-877103 ] regress: signedness confusion and code increase