From: SourceForge.net <no...@so...> - 2004-10-05 11:08:05
|
Bugs item #979599, was opened at 2004-06-25 10:44 Message generated for change (Comment added) made by maartenbrock You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=100599&aid=979599&group_id=599 Category: C-Front End Group: fixed Status: Closed Resolution: Fixed Priority: 5 Submitted By: Josef Pavlik (jetset) Assigned to: Bernhard Held (bernhardheld) Summary: inefficent/wrong code for PORT&=~mask Initial Comment: hello I found, that the expression PORT &= ~mask is not compiled efficently, in some cases this code can make unpredictable result. I expect, that the resulting code will be anl PORT, #~mask but the value of the port is loaded to the register, then anded with mask and is reloaded to the port. If the port value change or if this is some input signals on the port, the 0 read from input signal will be written as 0 to output and will block the input port. The expression PORT |= mask is compiled OK I'm using the last version of compiler (15.6.04) with --model-large see the piece of resulting code in attachment #define RS485_POWER_PORT P3 #define RS485_POWER_MASK 0x08 ;rs485.c:321: RS485_POWER_PORT|=RS485_POWER_MASK; ; genOr orl _P3,#0x08 ; Peephole 112.b changed ljmp to sjmp ; Peephole 251.b replaced sjmp to ret with ret ret 00102$: ;rs485.c:325: RS485_POWER_PORT&=~RS485_POWER_MASK; ; genAssign mov r2,_P3 ; genAnd mov a,#0xF7 anl a,r2 mov _P3,a 00104$: ---------------------------------------------------------------------- >Comment By: Maarten Brock (maartenbrock) Date: 2004-10-05 13:08 Message: Logged In: YES user_id=888171 New fix available in SDCC 2.4.5 #855 ---------------------------------------------------------------------- Comment By: Erik Petrich (epetrich) Date: 2004-09-15 07:10 Message: Logged In: YES user_id=635249 The latest optimization attempt causes this code to fail: volatile unsigned char c; unsigned char foo(void) { char c2; c2 = c; c = c2 & 0x01; return c2 & 0x02; } _foo: ;and.c:9: c = c2 & 0x01; ; genAnd anl _c,#0x01 ;and.c:10: return c2 & 0x02; ; genAnd mov a,#0x02 anl a,_c ; <<< current value instead of original value mov dpl,a ; genRet 00101$: ret /* Look for two subsequent iCodes with */ /* iTemp := _c; */ /* _c = iTemp & op; */ /* and replace them by */ /* _c = _c & op; */ This is only valid if iTemp has only a single use. Otherwise, the transformation must be: /* Look for two subsequent iCodes with */ /* iTemp := _c; */ /* _c = iTemp & op; */ /* and replace them by */ /* iTemp := _c; */ /* _c = _c & op; */ ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2004-09-12 18:44 Message: Logged In: YES user_id=888171 Applied Bernhards fix in SDCC 2.4.4 #838 ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2004-07-20 12:56 Message: Logged In: YES user_id=888171 Unfortunately, the fix introduced other bugs. So it is withdrawn. ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2004-07-19 22:03 Message: Logged In: YES user_id=888171 Bernhard, Thanks for reviewing my code and comments. It's a pity you didn't respond earlier to my comments here though. I know chars should be promoted to signed int, but is a constant "1" a char or an int? And is it signed or unsigned? So I tried to adhere to SDCC's policy to avoid promotion. You also mention not to cast to the type of the result, but my fix does not do that. It casts the signedness of literal _right_ to that of non-literal _left_. And _left_ cannot be a float only integer. 6.5 4 "Some operators (the unary operator ~, and the binary operators <<, >>, &, ^, and |, collectively described as bitwise operators) are required to have operands that have integer type. These operators return values that depend on the internal representations of integers, and have implementation-defined and undefined aspects for signed types." "unsigned int c; c &= ~1": In this case SDCC first creates a literal (not signed or unsigned) char (0x01). Then it applies ~ to it and makes the new literal signed char (0xFE). The & promotes this to unsigned int (0xFFFE, this is where the signed is used) and finally the code is produced to do the AND and store the result. AND-ing the high byte with 0xFF is optimized away in the proces. This is what SDCC did and still does. As you can deduce from this. For "unsigned long int d; d &= ~1" SDCC generates the literal unsigned long int (0xFFFFFFFE). You say this is against the standard.(?) My fix was not meant to optimize the case with no literal involved. It doesn't touch it. My philisophy was: if you want optimal code out of the compiler for "P1 &= ~mask", you have to help. So make mask unsigned yourself or expect all sorts of promotions. "l = i & 0x8000u": I agree, this seems to give the wrong result -32768, where it should be +32768 according to the standard. (0==0 is no problem) "Otherwise, if the operand that has unsigned integer type has rank greater or equal to the rank of the type of the other operand, then the operand with signed integer type is converted to the type of the operand with unsigned integer type." "i = c & 0x80": I don't think I've changed this. The result should be 128 or 0 according to the standard. I have not checked yet, but I guess SDCC already performed the promotion to int or that's another bug. Frieder, This was a recent change to try to conform bit behaviour to _Bool behaviour as described in C99. And we can only try to persuade Silabs to update the example to use ! as we agreed it should be done. I'll have a go at that. I'll look into this some more tomorrow, Maarten ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-07-19 21:13 Message: Logged In: YES user_id=203539 > LED = ~LED; > setb _LED is perfect correct, this has already been discussed several times. I knew this will become a FAQ :-( What you want is LED = !LED; ---------------------------------------------------------------------- Comment By: Frieder Ferlemann (frief) Date: 2004-07-19 18:11 Message: Logged In: YES user_id=589052 ~ on bit variables is problematic too. The example blinky.c for SDCC on the Silabs website (Application note AN198, Integrating SDCC 8051 Tools Into The Silicon Labs IDE:) uses the problematic construct: LED = ~LED; // change state of LED Which compiles to cpl _LED with SDCC 2.4.0 but to setb _LED with recent SDCC versions. ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-07-19 17:13 Message: Logged In: YES user_id=203539 3) Read section "6.3.1.8 Usual arithmetic conversions": chars should be promoted to signed int. It's not optimal to cast to the type of the result, if e.g. the result is of type float or long. But: SDCC tries to avoid the promotion, if the result is a char (or smaller). > And why does "unsigned int c; c &= ~1" generate highly > compact code? In this case ~1 IS unsigned although it > started out signed. I've to think about it. It's always possible to cast from 'signed int' to 'unsigned int' and vicy versa with on sweat. May be it's also possible to cast from 'signed char' to 'unsigned char' and vicy versa without getting into trouble with signedness and promotion rules. > I'm thinking of changing the patch for bug 524195. Instead > of hard casting ~ on literals signed, I guess this is buggy. > I suggest hard casting of char to int. This is certainly correct but not always optimal. > So the next question is if this is according to the > C-standard Yes. Regarding your "fix": Optimization in AST have generally the disadvantage that they can't catch optimizations in iCode: #include "8051.h" void foo (void) { char mask = 0x11; P1 &= ~mask; } Your "fix" doesn't help here, but my "hack" catches this optimization. Furthermore you introduced a bug: it's true that the AND operation is signless. But the signedness of the _result_ and promotion rules depend on the signedness of the operands. It's more than dangerous to change the signedness of an operand, especially at the AST level: int i; long l; void foo (void) { l = i & 0x8000u; } or char c; int i; void foo (void) { i = c & 0x80; } ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2004-07-11 12:46 Message: Logged In: YES user_id=888171 Fixed SDCCast.c 1.239 ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2004-07-08 10:18 Message: Logged In: YES user_id=888171 1) (unsigned) port &= unsigned is completely in the unsigned domain, but (unsigned) port &= signed needs a cast from (unsigned) port to signed port. It's this cast that stands in the way of optimization. 2) According to the C standard the constant should be signed int. But we have an 8-bit cpu and would like to keep as much arithmatic in a char. So the constant is (signed) char. 3) In section 6.5.3.3 the word (promoted) worries me most. To what should we promote? To the type of the operand? (ridiculous) To int (for char or bitfield)? To the type the result will be promoted to? (which seems best to me, but is harder to achieve) I hope you agree this standard is a pain to read. I'll go and have a look if I can manage the last one. ---------------------------------------------------------------------- Comment By: Josef Pavlik (jetset) Date: 2004-07-07 02:32 Message: Logged In: YES user_id=756222 one question: Why the code of port &= unsigned is different of port &= signed? another question: the constant should be signed or unsigned? reading the http://www.open-std.org/jtc1/sc22/wg14/www/docs/n843.htm section 6.4.4.1 I found the following: type of an decimal constant w/o suffix is the first enought from the following list: signed int, signed long int, signed long long int type of hex and oct constant w/o suffix is the first enought from the following list: signed int, unsigned int, signed long int, unsigned long int, signed ll int, unsigned ll int (interesting thing: the constants 32768 and 0x8000 are not equal in 16bit CPU! ) Seems that the constat should be signed. before the aritmetic operation is done, the operands should be converted using the following rules: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n843.htm section 6.3.1.7 The integer promotions are performed on both operands. Then the following rules are applied to the promoted operands: If both operands have the same type, then no further conversion is needed. Otherwise, if both operands have signed integer types or both have unsigned integer types, the operand with the type of lesser integer conversion rank is converted to the type of the operand with greater rank. Otherwise, if the operand that has unsigned integer type has rank greater or equal to the rank of the type of the other operand, then the operand with signed integer type is converted to the type of the operand with unsigned integer type. Otherwise, if the type of the operand with signed integer type can represent all of the values of the type of the operand with unsigned integer type, then the operand with unsigned integer type is converted to the type of the operand with signed integer type. Otherwise, both operands are converted to the unsigned integer type corresponding to the type of the operand with signed integer type. And finally, section 6.5.3.3 says: [#4] The result of the ~ operator is the bitwise complement | of its (promoted) operand (that is, each bit in the result is set if and only if the corresponding bit in the converted operand is not set). The integer promotions are performed on the operand, and the result has the promoted type. If | the promoted type is an unsigned type, the expression ~E is | equivalent to the maximum value representable in that type | minus E. I hope, that those informations can help you to resolve the problem. Jet ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2004-07-06 22:06 Message: Logged In: YES user_id=888171 That should have been: unsigned long int d; ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2004-07-06 21:59 Message: Logged In: YES user_id=888171 Josef, You're right. It's all about the bitwise complement operator which renders the constant signed. And somehow it's never changed to unsigned again. Bernhard's fix helps, but it feels a bit like a hack. What if something else comes in between... And why does "unsigned int c; c &= ~1" generate highly compact code? In this case ~1 IS unsigned although it started out signed. I'm thinking of changing the patch for bug 524195. Instead of hard casting ~ on literals signed, I suggest hard casting of char to int. This seems to generate compact and correct code, even when applied to int's. But if used with long ints it generates this code: long int d; d &= ~1; anl _d,#0xFE mov (_d + 2),#0x00 mov (_d + 3),#0x00 So the next question is if this is according to the C-standard or not? Of course ~1L gives the expected result. Maarten ---------------------------------------------------------------------- Comment By: Josef Pavlik (jetset) Date: 2004-07-01 19:58 Message: Logged In: YES user_id=756222 I mean the bitwise complement of the mask which is evaluated by the compiler. Example: the expression port|=~mask versus port|=mask ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-07-01 16:46 Message: Logged In: YES user_id=203539 ~ is the "bitwise complement". It still can't see any problem. The only opcode of the 8051 with complement is "cpl a", therefore SDCC can never generate a read-modify-write code for the complement operation. ---------------------------------------------------------------------- Comment By: Josef Pavlik (jetset) Date: 2004-07-01 00:44 Message: Logged In: YES user_id=756222 sorry, I mean the ~ operator. You're right. 'not' is '!'. what is the tilde? Complement? Negate? Unfortunately, English is not my best known language :-((( ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-06-30 22:42 Message: Logged In: YES user_id=203539 Both bitwise or and xor are already included in my patch. What tests did you do with the "NOT statement"? There's no read-modify-write operation with logical negation, neither in C nor in 8051 assembler. '!=' looks similiar to '^=' but the semantic is somewhat different ;-) ---------------------------------------------------------------------- Comment By: Josef Pavlik (jetset) Date: 2004-06-30 00:13 Message: Logged In: YES user_id=756222 Reading the comments and making additional test I found, that the problem is not bound to the AND statement, but to the NOT statement and is present in the following two cases too: port|=~mask port^=~mask ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-06-29 22:41 Message: Logged In: YES user_id=203539 The answer is easy: there's no code for this optimization. I've attached my fix. The optimization itself is ok, but I'm in doubt about the liferange-part. I never touched the LR code and I wasn't able to test it. I would be happy if somebody could review my code. ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-06-29 22:41 Message: Logged In: YES user_id=203539 The answer is easy: there's no code for this optimization. I've attached my fix. The optimization itself is ok, but I'm in doubt about the liferange-part. I never touched the LR code and I wasn't able to test it. I would be happy if somebody could review my code. ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2004-06-29 20:58 Message: Logged In: YES user_id=888171 The fix for bug 524195 is partly responsible for this 'bug'. See SDCCval.c 1.52. Essentially the result from ~ is always signed. Now the question remains why (signed)iTemp0 = (unsigned)c (signed)iTemp1 = (signed)iTemp0 & (signed)0xF7 (unsigned)c = (signed)iTemp0 is not optimized. ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2004-06-29 10:30 Message: Logged In: YES user_id=888171 The volatileness of P3 has nothing to do with it. As you found out P3 &= (unsigned char)~0x08 does give optimized output. Replacing P3 by 'volatile unsigned char c' gives the same result as well as 'unsigned char c'. Even this does not generate optimal code: c &= ~0x08U So it must be some conversion from unsigned to signed by ~ that gets in the way. When c is signed it always generates optimal code, volatile or not. ---------------------------------------------------------------------- Comment By: Bernhard Held (bernhardheld) Date: 2004-06-28 09:33 Message: Logged In: YES user_id=203539 Another quick fix is PORT &= (unsigned char) ~mask; I've already spent some time on this issue. The beginning of the problem is a cast from 'signed char' to 'unsigned char' added in iCode. This cast should be eliminated by the optimizer (findCheaperOp()). The problem here is, that P3 is volatile, and the optimizer refuses to touch this iCode (usigned char c; c &= ~mask; // isn't optimized too!?). It's not (yet) possible to avoid the addition of the cast, because it's important to keep the signedness of all operands. Even in mcs51/gen.c the signedness of an operation is still determined by the signedness of the operands. IMO the signedness of the operation should be determined in AST and used throughout the iCode and code generators. This would generally enable more aggressive optimizations of operands. Finally I also planed to fix this issue with the peephole optimizer, but Frieder pointed out an important problem: the register and acc isn't updated. It seems to be necessary to add another special case to the optimizer (mcs51/packRegsForAssign()). ---------------------------------------------------------------------- Comment By: Josef Pavlik (jetset) Date: 2004-06-28 01:15 Message: Logged In: YES user_id=756222 Hi Maarten, the behaviour, that read-modify-write instructions reads the output register instead of real state of the port is the standard behaviour of the original 8051 (c) Intel. Thanx to Frief for the ad-hoc work around Jet ---------------------------------------------------------------------- Comment By: Frieder Ferlemann (frief) Date: 2004-06-27 19:40 Message: Logged In: YES user_id=589052 The attached file has a temporary fix for the problem to get things going until it is fixed properly To make use of this additionally specify "--peep-file peeph_979599.def " on the sdcc command line. Note: this peephole still updates r2 and acc although both of them probably won't hold the expected values (as you and Maarten pointed out). ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2004-06-27 14:13 Message: Logged In: YES user_id=888171 I'm not sure what an original 8051 does but at least some Silabs derivatives behave differently between ANL _P3,#0x08 and MOV A,_P3, ANL A,#0x08, MOV _P3,A. This is from their documentation: A Read of a Port Data register (or Port bit) will always return the logic state present at the pin itself, regardless of whether the Crossbar has allocated the pin for peripheral use or not. An exception to this occurs during the execution of a read- modify-write instruction (ANL, ORL, XRL, CPL, INC, DEC, DJNZ, JBC, CLR, SET, and the bitwise MOV operation). During the read cycle of the read-modify-write instruction, it is the contents of the Port Data register, not the state of the Port pins themselves, which is read. And you're right IMHO that you may assume the direct code to be produced. I know I would. Greets, Maarten ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=100599&aid=979599&group_id=599 |