Re: [Rapp-dev] SSSE3 alignment and rapp_validate_alignment

[Willie Betschart]

Hi H-P

Sure I will deliver in separate messages if I would have more than one updates 
at the time.

I have updated the code after the review comments.

The second patch was needed to be created when I tested the patch. I had a 
problem with test/Makefile.in file, Makefile.am is correct but wasn't 
regenerated, strange. I wasn't sure if this is related to my reinstalled 
computer or if is a problem with checked in autogenerated files with autoreconf.

Yes this was intended for driver development but we have identified the 
importance in several fields.

Regarding SSSE3, we don't use that backend since SSE2 is faster, except for 
those operations with alignment. I found the bug when reviewing something else.
I got about the same results as before, SSE2 was faster than SSSE3 when I 
retried to perform tuning on both backends. I really thought SSSE3 was an 
extention to SSE2.


Best regards
Willie


________________________________________
Från: Hans-Peter Nilsson address@hidden
Skickat: den 20 februari 2014 16:42
Till: Willie Betschart
Kopia: address@hidden
Ämne: Re: [Rapp-dev] SSSE3 alignment and rapp_validate_alignment

> From: Willie Betschart <address@hidden>
> Date: Wed, 19 Feb 2014 11:17:53 +0100

> I deliver two patches to RAPP

Note for the future: separate messages are preferred for
separate issues.

(Also, the test-case could be a separate patch since it tests
existing functions.  The rapp_validate_* patch could then build
on that.)

> Correct-alignment-for-SSSE3.patch sets an alignment of 16 bytes.

Oops.  How did we not see this before?  Thanks.

Though this bug is in the original code set, this also
highlights a porting-pitfall: you have to tweak this code too,
for a new port, which is easily overlooked and may have a
somewhat hairy failure mode.



> Tested with
> ./configure --enable-backend=ssse3
> make check

I'm curious: have you observed it failing?
Was it from the new test or was it "just" code inspection?

> rapp_validate_alignment.patch

> This adds a new API, mainly for validating buffers allocated
> form external sources, i.e. OpenCV buffers.

...or rather, buffers from e.g. drivers...

> Example
> if(rapp_validate_alignment(buf, ....) == 0){
>    new_buf = rapp_malloc()
>    copy(new_buf, buf)
>    free(buf)
>    buf = new_buf
> }

Beware of the bug in that example: you need to hold on to the
old buffer when it can't be free'd with "rapp_free()".

At first I thought "why, what's the benefit of this function
compared to (((uintptr_t) buf) & (rapp_alignment - 1))" but then
I looked at the patch and noticed that the function *does* have
value on its own, but is misnamed; it validates not only the
alignment but also the easily-overlooked buffer parameters (also
should be pointed out in its doxygen comments).  Thus, I think
it would be better named "rapp_validate_buffer"; the parameter
checking is of value for buffers built from rapp_malloc too.

The patch itself has issues in using a different formatting
style than other RAPP code, for example missing space before
if-argument parentheses and the code-block opening bracket (see
your code example); comment formatting (should also be full
sentences) and for using "//" for comments.

Patches should always follow the existing coding conventions.
There are other issues, please take care.  As for any project,
double-check with nearby code and for this project we even have
it in a file called HACKING.  And the test-suite header seems
odd; it tests several things (which is nice), not just
rapp_malloc.  As mentioned, that could go in separately.

I'm on the fence here: the API would change, but we don't add
any new core function.  Though it'd be a nice convenience, all
the functionality is already there; the user "just" has to
remember to check not just the buffer pointer (with an
expression like the one above) but also the "dimensions" of the
buffer.  So, I think the function should go in only together
with other new functionality that affects the API, to avoid a
version bump just for a convenience function.

Thanks.

brgds, H-P

0001-New-API-Validation-of-buffer-allocation.patch
Description: 0001-New-API-Validation-of-buffer-allocation.patch

0002-New-API-Validation-of-buffer-allocation.patch
Description: 0002-New-API-Validation-of-buffer-allocation.patch