[SLOF] [PATCH 4/4] fbuffer: Implement RFILL as an accelerated primitive

[Thomas Huth]

On 31/07/15 22:18, Segher Boessenkool wrote:
> Hi Thomas, a few comments...
> 
> On Fri, Jul 31, 2015 at 03:00:08PM +0200, Thomas Huth wrote:
>> +#define _RFILL(dst, size, pat, t)	\
> 
> Not new in your patch, but underscore-capital is a reserved name.

Yeah, I know, I just tried to adapt to the coding style of the other
macros in that file ... well, maybe it's time for a change now to avoid
continuing the sins from the past...

>> +{ \
>> +	t *d1 = (t *)dst; \
>> +	register t tmp = 0; \
>> +	int i = sizeof(t); \
>> +	while (i > 0) { tmp <<= 8; tmp |= pat & 0xff; } \
>> +	SET_CI; \
>> +	while (size > 0) { \
>> +		*d1++ = tmp; size -= sizeof(t); \
>> +	} \
>> +	CLR_CI; \
>> +}
> 
> You write the first block on one line, and the second on separate lines --
> pick one :-)

Ok, will do.

> "register" is useless.

... apart from the fact that you can not take the address of a register
variable with the "&" operator and thus gain a little bit more hope that
the compiler puts the variable into a register.

Apart from that, I know, the compiler can ignore this completely ... but
it gives me a better feeling when I write it there ;-)

To be really sure that we do not touch the normal memory between the
SET_CI and CLR_CI, we would otherwise have to write everything with
inline assembly ... but that's less readable. And since I doubt that the
compiler will ever generate such a bad code here ever, I think it should
be fine here without inline assembly code, too.

> You might need "unsigned" there though, or is "t" always unsigned already?

The used types are unsigned already, see slof/types.h.

> You shouldn't need that (t *) cast.

Ok, should be fine for the current code - until somebody tries to use
the macro with a different kind of dst variable.

>> +#define _FASTRFILL(dst, size, pat) \
>> +	switch (((type_u)dst | size) & (sizeof(type_u)-1)) { \
>> +		case 0:			_RFILL(dst, size, pat, type_u); break; \
>> +		case sizeof(type_l):	_RFILL(dst, size, pat, type_l); break; \
>> +		case sizeof(type_w):	_RFILL(dst, size, pat, type_w); break; \
>> +		default:		_RFILL(dst, size, pat, type_c); break; \
>> +	}
> 
> This doesn't handle case 6 as "w".

True ... actually, I've just copy-n-pasted this code from _FASTRMOVE
which also lacks this problem!

>> +PRIM(RFILL)
>> +	type_u pat = ((dp--)->u);
>> +	type_u size = ((dp--)->u);
>> +	type_u *dst = (type_u *)((dp--)->u);
> 
> type_u *dst = (dp--)->a;

Right ... again, I've just copy-n-pasted this code from PRIM(RMOVE)
which has the same issue.

Thanks for the review!

 Thomas