As you may know, the CCodeGenerator is clever enough to #generateBitShift:on:indent: with proper direction << or >> when the shift is literal. When the shift in (expr bitShift: shift) is a variable or another expression, it will produce a runtime test (shift <0) ? expr>>shift : expr <<shift; This is good for an arbitrary shift, but more than often we know the sign and direction of the shift in advance, and this runtime test is void. We must be aware that aggressive inlining will spread these useless code all around. Well, we might ignore impact on performance but we shall better not ignore warnings generated by C compiler (or XCode analyzer) if we want the code to be portable to other compilers (including future versions of gcc). ...and the eventual warnings will be spreaded too
Maybe we could arrange for CCodeGenerator to recognize more cases when the shift is unsigned, but 1) the CCodeGenerator is yet unable to infer expression type (except some hack for declared variable) 2) we almost never use unsigned declaration in vm source anyway.
In the mean time, I suggest we use << and >> directlty in smalltalk code (slang) when we know the direction in advance.
Find a few examples attached
Nicolas
On Sun, Dec 23, 2012 at 02:14:37PM +0100, Nicolas Cellier wrote:
As you may know, the CCodeGenerator is clever enough to #generateBitShift:on:indent: with proper direction << or >> when the shift is literal. When the shift in (expr bitShift: shift) is a variable or another expression, it will produce a runtime test (shift <0) ? expr>>shift : expr <<shift; This is good for an arbitrary shift, but more than often we know the sign and direction of the shift in advance, and this runtime test is void. We must be aware that aggressive inlining will spread these useless code all around. Well, we might ignore impact on performance but we shall better not ignore warnings generated by C compiler (or XCode analyzer) if we want the code to be portable to other compilers (including future versions of gcc). ...and the eventual warnings will be spreaded too
Maybe we could arrange for CCodeGenerator to recognize more cases when the shift is unsigned, but
- the CCodeGenerator is yet unable to infer expression type (except
some hack for declared variable) 2) we almost never use unsigned declaration in vm source anyway.
In the mean time, I suggest we use << and >> directlty in smalltalk code (slang) when we know the direction in advance.
IMO it is better to write these expressions directly, as you have done in your examples. There are many places in the VM code where types are not carefully declared, and I suspect that trying to make the code generator be more clever would only lead to unintended side effects. Better to just write these explicitly using << and >> when you know that it is safe to do so.
Find a few examples attached
Nicolas
These look like good improvements to me. To show the effect, here is what the generated code looks like (VMM trunk) for two of your patches:
=== JPEGReaderPlugin>>getBits: ===
Current generated code:
static sqInt getBits(sqInt requestedBits) { sqInt value;
if (requestedBits > jsBitCount) { fillBuffer(); if (requestedBits > jsBitCount) { return -1; } } value = (((requestedBits - jsBitCount) < 0) ? ((usqInt) jsBitBuffer >> -(requestedBits - jsBitCount)) : ((usqInt) jsBitBuffer << (requestedBits - jsBitCount))); jsBitBuffer = jsBitBuffer & (((((jsBitCount - requestedBits) < 0) ? ((usqInt) 1 >> -(jsBitCount - requestedBits)) : ((usqInt) 1 << (jsBitCount - requestedBits)))) - 1); jsBitCount -= requestedBits; return value; }
After applying the patch:
static sqInt getBits(sqInt requestedBits) { sqInt value;
if (requestedBits > jsBitCount) { fillBuffer(); if (requestedBits > jsBitCount) { return -1; } } jsBitCount -= requestedBits; value = ((usqInt) jsBitBuffer) >> jsBitCount; jsBitBuffer = jsBitBuffer & ((1 << jsBitCount) - 1); return value; }
=== JPEGReaderPlugin>>scaleAndSignExtend:inFieldWidth: ===
Current generated code:
static sqInt scaleAndSignExtendinFieldWidth(sqInt aNumber, sqInt w) { if (aNumber < ((((w - 1) < 0) ? ((usqInt) 1 >> -(w - 1)) : ((usqInt) 1 << (w - 1))))) { return (aNumber - (((w < 0) ? ((usqInt) 1 >> -w) : ((usqInt) 1 << w)))) + 1; } else { return aNumber; } }
After applying the patch:
static sqInt scaleAndSignExtendinFieldWidth(sqInt aNumber, sqInt w) { if (aNumber < (1 << (w - 1))) { return (aNumber - (1 << w)) + 1; } else { return aNumber; } }
On 23 December 2012 18:05, David T. Lewis lewis@mail.msen.com wrote:
On Sun, Dec 23, 2012 at 02:14:37PM +0100, Nicolas Cellier wrote:
As you may know, the CCodeGenerator is clever enough to #generateBitShift:on:indent: with proper direction << or >> when the shift is literal. When the shift in (expr bitShift: shift) is a variable or another expression, it will produce a runtime test (shift <0) ? expr>>shift : expr <<shift; This is good for an arbitrary shift, but more than often we know the sign and direction of the shift in advance, and this runtime test is void. We must be aware that aggressive inlining will spread these useless code all around. Well, we might ignore impact on performance but we shall better not ignore warnings generated by C compiler (or XCode analyzer) if we want the code to be portable to other compilers (including future versions of gcc). ...and the eventual warnings will be spreaded too
Maybe we could arrange for CCodeGenerator to recognize more cases when the shift is unsigned, but
- the CCodeGenerator is yet unable to infer expression type (except
some hack for declared variable) 2) we almost never use unsigned declaration in vm source anyway.
In the mean time, I suggest we use << and >> directlty in smalltalk code (slang) when we know the direction in advance.
IMO it is better to write these expressions directly, as you have done in your examples. There are many places in the VM code where types are not carefully declared, and I suspect that trying to make the code generator be more clever would only lead to unintended side effects. Better to just write these explicitly using << and >> when you know that it is safe to do so.
+1
Another candidate in the same category of refactoring..,
2012/12/23 Igor Stasenko siguctua@gmail.com:
On 23 December 2012 18:05, David T. Lewis lewis@mail.msen.com wrote:
On Sun, Dec 23, 2012 at 02:14:37PM +0100, Nicolas Cellier wrote:
As you may know, the CCodeGenerator is clever enough to #generateBitShift:on:indent: with proper direction << or >> when the shift is literal. When the shift in (expr bitShift: shift) is a variable or another expression, it will produce a runtime test (shift <0) ? expr>>shift : expr <<shift; This is good for an arbitrary shift, but more than often we know the sign and direction of the shift in advance, and this runtime test is void. We must be aware that aggressive inlining will spread these useless code all around. Well, we might ignore impact on performance but we shall better not ignore warnings generated by C compiler (or XCode analyzer) if we want the code to be portable to other compilers (including future versions of gcc). ...and the eventual warnings will be spreaded too
Maybe we could arrange for CCodeGenerator to recognize more cases when the shift is unsigned, but
- the CCodeGenerator is yet unable to infer expression type (except
some hack for declared variable) 2) we almost never use unsigned declaration in vm source anyway.
In the mean time, I suggest we use << and >> directlty in smalltalk code (slang) when we know the direction in advance.
IMO it is better to write these expressions directly, as you have done in your examples. There are many places in the VM code where types are not carefully declared, and I suspect that trying to make the code generator be more clever would only lead to unintended side effects. Better to just write these explicitly using << and >> when you know that it is safe to do so.
+1
-- Best regards, Igor Stasenko.
I have opened http://code.google.com/p/cog/issues/detail?id=111 with proposed .cs to review (for .oscog branch)
Nicolas
2012/12/27 Nicolas Cellier nicolas.cellier.aka.nice@gmail.com:
Another candidate in the same category of refactoring..,
2012/12/23 Igor Stasenko siguctua@gmail.com:
On 23 December 2012 18:05, David T. Lewis lewis@mail.msen.com wrote:
On Sun, Dec 23, 2012 at 02:14:37PM +0100, Nicolas Cellier wrote:
As you may know, the CCodeGenerator is clever enough to #generateBitShift:on:indent: with proper direction << or >> when the shift is literal. When the shift in (expr bitShift: shift) is a variable or another expression, it will produce a runtime test (shift <0) ? expr>>shift : expr <<shift; This is good for an arbitrary shift, but more than often we know the sign and direction of the shift in advance, and this runtime test is void. We must be aware that aggressive inlining will spread these useless code all around. Well, we might ignore impact on performance but we shall better not ignore warnings generated by C compiler (or XCode analyzer) if we want the code to be portable to other compilers (including future versions of gcc). ...and the eventual warnings will be spreaded too
Maybe we could arrange for CCodeGenerator to recognize more cases when the shift is unsigned, but
- the CCodeGenerator is yet unable to infer expression type (except
some hack for declared variable) 2) we almost never use unsigned declaration in vm source anyway.
In the mean time, I suggest we use << and >> directlty in smalltalk code (slang) when we know the direction in advance.
IMO it is better to write these expressions directly, as you have done in your examples. There are many places in the VM code where types are not carefully declared, and I suspect that trying to make the code generator be more clever would only lead to unintended side effects. Better to just write these explicitly using << and >> when you know that it is safe to do so.
+1
-- Best regards, Igor Stasenko.
On Fri, Dec 28, 2012 at 12:47:40AM +0100, Nicolas Cellier wrote:
I have opened http://code.google.com/p/cog/issues/detail?id=111 with proposed .cs to review (for .oscog branch)
Nicolas
Thanks Nicolas,
The Sound_replace_bitShift_with_directedShift.cs change set is now added to Squeak trunk, package Sound. This makes the ADPCMCodecPlugin updates for any VM (Cog or interpreter) built from an updated Squeak trunk image. This change set should also be added to Pharo.
I added VMMaker_replace_bitShift_with_directedShift.cs to VMMaker trunk for the interpreter VM. This should also be added to the oscog branch of VMMaker for Cog.
BTW let me know if you want to be added as a developer on VMMaker.
Dave
2012/12/27 Nicolas Cellier nicolas.cellier.aka.nice@gmail.com:
Another candidate in the same category of refactoring..,
2012/12/23 Igor Stasenko siguctua@gmail.com:
On 23 December 2012 18:05, David T. Lewis lewis@mail.msen.com wrote:
On Sun, Dec 23, 2012 at 02:14:37PM +0100, Nicolas Cellier wrote:
As you may know, the CCodeGenerator is clever enough to #generateBitShift:on:indent: with proper direction << or >> when the shift is literal. When the shift in (expr bitShift: shift) is a variable or another expression, it will produce a runtime test (shift <0) ? expr>>shift : expr <<shift; This is good for an arbitrary shift, but more than often we know the sign and direction of the shift in advance, and this runtime test is void. We must be aware that aggressive inlining will spread these useless code all around. Well, we might ignore impact on performance but we shall better not ignore warnings generated by C compiler (or XCode analyzer) if we want the code to be portable to other compilers (including future versions of gcc). ...and the eventual warnings will be spreaded too
Maybe we could arrange for CCodeGenerator to recognize more cases when the shift is unsigned, but
- the CCodeGenerator is yet unable to infer expression type (except
some hack for declared variable) 2) we almost never use unsigned declaration in vm source anyway.
In the mean time, I suggest we use << and >> directlty in smalltalk code (slang) when we know the direction in advance.
IMO it is better to write these expressions directly, as you have done in your examples. There are many places in the VM code where types are not carefully declared, and I suspect that trying to make the code generator be more clever would only lead to unintended side effects. Better to just write these explicitly using << and >> when you know that it is safe to do so.
+1
-- Best regards, Igor Stasenko.
vm-dev@lists.squeakfoundation.org