[Vm-dev] bitShift: and runtime sign discussion

[David T. Lewis]

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
> 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.

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;
	}
}