From: SourceForge.net <no...@so...> - 2004-01-12 17:57:24
|
Bugs item #832664, was opened at 2003-10-30 00:02 Message generated for change (Comment added) made by stsp You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=100599&aid=832664&group_id=599 Category: Icode generator Group: fixed Status: Closed Resolution: Fixed Priority: 5 Submitted By: Stas Sergeev (stsp) Assigned to: Bernhard Held (bernhardheld) Summary: missing type propagation to int Initial Comment: Hi. There seem to be a bug which causes a data loses during the evaluation of some complex expressions. The program that demonstrates the bug, is attached. Also here it is: --- #include <at89S8252.h> int c; unsigned char p; int main() { p = SBUF + 13; c = 2 | ((int)1 << p); return c; } --- The result is that c==2. The asm code in question follows: --- ;shift.c:9: c = 2 | ((int)1 << p); ; genLeftShift mov b,_p inc b mov r2,#0x01 mov r3,#0x00 sjmp 00104$ 00103$: mov a,r2 add a,acc mov r2,a mov a,r3 rlc a mov r3,a 00104$: djnz b,00103$ ; genCast --> There might be no cast to byte <--- ; genOr orl ar2,#0x02 ; genCast mov _c,r2 mov (_c + 1),#0x00 --> so it was casted, the higher byte is lost <--- ;shift.c:10: return c; --- The strange thing here is that the expression c = (1 << p); gets evaluated correctly, but c = (int)2 | (1 << p); is not and only c = (int)2 | ((int)1 << p); is evaluated correctly. What puzzles me is that adding "2 |" affects the evaluation of (1 << p) making it to loose the higher byte, while it seems logical to expect that these expressions are independant. ---------------------------------------------------------------------- >Comment By: Stas Sergeev (stsp) Date: 2004-01-12 20:57 Message: Logged In: YES user_id=501371 Ouch, it looks like I can't type the certain characters here, in particular the < sign (or two < signs in a row). Trying again... sorry about a spam. It seems your patch always evaluates a left shift as an int. Even the simple shifting expressions with all args and the result being char, are now evaluated via ints. Perhaps it is possible to evaluate the expression as a char in case all the args and the result are char's? My program is doing an enormous amount of shifting stuff, but 99% of it is done with "unsigned char" args and result. Now they all are evaluated via ints, which adds a lot. Would it be possible to do something about it? ---------------------------------------------------------------------- Comment By: Stas Sergeev (stsp) Date: 2004-01-12 20:50 Message: Logged In: YES user_id=501371 Hmm, why the previous follow-up got dropped? Retrying. It seems your patch always calculates a left shift as an int. Even a=1<<b is calculated as int, while both "a" and "b" are the "unsigned char"s. Perhaps it would help to not use int in case the result and all the arguments in an expression are the "unsigned char"s? My program is doing an enormous amount of a shifting stuff, but 99% of it are done with the "unsigned char"s. Now they all are done via ints, which adds a lot. Would it be possible to do something/anything/whatever about that? ---------------------------------------------------------------------- Comment By: Stas Sergeev (stsp) Date: 2004-01-12 20:43 Message: Logged In: YES user_id=501371 It seems your patch always calculates a left shift as an int. Even a=1<<b is now evaluated as int, while both "a" and "b" are "unsigned char"s. Perhaps it is possible to do that as a char in case the result is char? I know there are some fundamental problems with that, but at least the trivial cases can be optimized. My program does an enormous amount of a shifting stuff, although 99% of it is done with the "unsigned char" arguments and result. Now it all is suddenly done via ints, then the growth. Would it be possible to do something/anything/whatever about that? ---------------------------------------------------------------------- Comment By: Stas Sergeev (stsp) Date: 2004-01-12 07:11 Message: Logged In: YES user_id=501371 OK, so this is not expected, good. Then there is a hope. I'll try to figure out more. ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-01-11 23:15 Message: Logged In: YES user_id=203539 My patch generally casts the left operator of a left shift operation to at least 'signed int'. This operation costs only few bytes. I've no idea about the reason for the 13% increase of your code size. You've to supply more information. ---------------------------------------------------------------------- Comment By: Stas Sergeev (stsp) Date: 2004-01-11 20:17 Message: Logged In: YES user_id=501371 Hello. I've finally got around to update from CVS and test the fix. Yes, indeed the patch works as expected, thanks. But... but it also works not as expected. Actually I am sorry to say that, but it is unacceptable for my needs. My program was ~7300bytes before the patch. With the patch applied, the program have grown up to ~8400 bytes and no longer fits into the flash. Well, giving up more than 1100 bytes is not possible for me. I realize your patch is (hopefully) correct and does what I demanded myself. But now I can't use it, no matter whether I want to or not. Very sad. I don't know what to do now. Perhaps I'll have to no longer upgrade sdcc and stick with the last version that didn't have that fix. Maybe you have some hints as to how can I solve that problem? If you perhaps need a testcases, I can try to create a few, but it will show nothing even nearly compared to 1100 bytes losses... ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-01-08 16:48 Message: Logged In: YES user_id=203539 Fixed, see ChangeLog 1.563 ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2003-11-01 15:57 Message: Logged In: YES user_id=203539 c = (1 << p); is evaluated correctly, because in decorateType() is a very special "band aid" to fix this case. It works only for casts and assignments, and if the right value is the result from +, -, * or <<. It doesn't even look at the next node of the tree. In this special case type flows up one single node. In order to fix (and all other cases) c = (int)2 | ((int)1 << p); type has to flow up the whole tree. ---------------------------------------------------------------------- Comment By: Stas Sergeev (stsp) Date: 2003-11-01 13:16 Message: Logged In: YES user_id=501371 > SDCC tries to use 8 bit values as far as possible. > Therefore the constants 1 and 2 are "signed char". Yes, I suspected that, and from that point of view the result looks perfectly correct. What made me wonder however, is the fact that "c = 1 << p" gives a non-zero result, which means to me that in that case it automatically promotes the expression to int for some reasons, and then I don't see the reason of not doing the same when I add "2 |" to the expression. But why "c = (int)2 | (1 << p)" still gives the wrong result? Isn't that a different bug? I mean, if I cast 2 to int explicitly, and "c = 1 << p" is evaluated as int, then why they in combination gives a char? ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2003-11-01 00:53 Message: Logged In: YES user_id=203539 You're right, it's a bug. SDCC tries to use 8 bit values as far as possible. Therefore the constants 1 and 2 are "signed char". At least SDCC is doing the whole computation in 8 bit only (see the output with --dumptree). It would be easy to change everything to "int", SDCC would perfectly comply with the standard. But I'm sure, that dozens of users will complain the next day about the unacceptable increase of memory consumption ... The real bug is, that type flows only bottom up (decorateType()). A type propagation, which flows the other way round, is still missing. I've already faced the problem 2 weeks ago, but there's still a far way to go. A lot of code will be necessary to handle many exceptions. I've re-opened your report to have a reminder for this special case. ---------------------------------------------------------------------- Comment By: Stas Sergeev (stsp) Date: 2003-10-31 23:52 Message: Logged In: YES user_id=501371 Thanks for the fix, although I think it is not complete, or at least there is something fishy about it. It fixes the original test-case, but if I change the line c = 2 | ((int)1 << p); to c = 2 | (1 << p); the result is that c==2 again. I think this is not correct. Also considering that "c = 1 << p" gives the proper value, not zero, I suspect the bug is still there. Am I missing something? ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2003-10-31 18:46 Message: Logged In: YES user_id=203539 Fixed in SDCCast.c 1.193 ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=100599&aid=832664&group_id=599 |