From: SourceForge.net <no...@so...> - 2010-05-07 17:15:42
|
Patches item #2997777, was opened at 2010-05-06 19:52 Message generated for change (Comment added) made by borutr You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=300599&aid=2997777&group_id=599 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Closed Resolution: Rejected Priority: 5 Private: No Submitted By: C. Scott Ananian (cananian) Assigned to: Nobody/Anonymous (nobody) Summary: Doesn't strength reduce signed division by powers of two. Initial Comment: SDCC correctly transforms: a /= 2 to a >>= 1 if a is unsigned, but doesn't perform this valid transformation when a is signed (in which case the sign bit is correctly propagated by the right shift). The attached patch fixes the problem. ---------------------------------------------------------------------- >Comment By: Borut Ražem (borutr) Date: 2010-05-07 19:15 Message: Maybe we should open a RFE to optimize division by power of 2 so that it will remain open and maybe someone will implement it someday.This one is already closed and will be lost in the time and space... ---------------------------------------------------------------------- Comment By: Borut Ražem (borutr) Date: 2010-05-07 19:08 Message: Exactly. And they showed the the patch is not correct since RIGHT_OP is implemented with logical shifts. ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2010-05-07 17:38 Message: Oh wait, I think you meant we have a test for division by two on signed integers. ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2010-05-07 17:36 Message: At least the Z80 and the HC08 have both logical and arithmetic shift right instructions. The mcs51 and ds390 have neither, they can only rotate. I know nothing about PICs. So we have regression tests to test undefined behaviour. You learn something new every day ;-) I agree to keep things as they are and not introduce a new operator for this optimization. Btw. I moved this item to PATCHES as it is no BUG and contained a patch. ---------------------------------------------------------------------- Comment By: Borut Ražem (borutr) Date: 2010-05-07 16:28 Message: It might work, but it doesn't: the RIGHT_OP is implemented as logical shift on all platforms (you can see regression tests failures if you apply the patch, as I did ;-). So Marten, you are right, there is a fourth possibility: - implement RIGHT_OP as arithmetic right shift instead of logical shift for all targets but this would break the backward compatibility in case somebody relay on this behavior and bloat the generated code for C shift (x >> n) on targets that doesn't natively support arithmetic shift right (do we have such one?) Borut ---------------------------------------------------------------------- Comment By: Maarten Brock (maartenbrock) Date: 2010-05-07 11:12 Message: Not quite. It might work for negative values as the behaviour is undefined in the C spec. I don't know what the current behaviour is but we could choose to make it an arithmetic shift. In general an arithmetic shift might be more expensive than a logical shift though if the mcu does support it natively. Maarten ---------------------------------------------------------------------- Comment By: Borut Ražem (borutr) Date: 2010-05-07 00:03 Message: It doesn't work for negative values, since the arithmetic shift should be used instead of logical shift produced by RIGHT_OP. I see 3 possible solutions: - introduce a new "arithmetic shift right" iCode operator (ASR_OP) - "manually" adjust the sign bit after shifting. This one probably doesn't make sense... - move the optimization from iCode level to target code generation level Borut ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=300599&aid=2997777&group_id=599 |