[Bug 217771] lang/beignet: update to 1.3.1

[bugzilla-noreply at freebsd.org]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=217771

--- Comment #8 from Matthew Rezny <rezny at freebsd.org> ---
(In reply to Jan Beich (mail not working) from comment #7)

>OpenCL 2.0 requires LLVM >= 3.9 and libdrm >= 2.4.67 (i.e. ports r431463 and pkg info libdrm
>maybe[1] drm-next). I forgot to adjust at least BUILD_DEPENDS in my version.

The LLVM requirement was bumped to 3.9 during the prior update to 1.3.0 and we
have had an up to date libdrm long enough that should be no concern either.

>[1] Whether drm_intel_bo_set_softpin_offset() works on FreeBSD 10.3 or 11.0 or causes a crash similar to bug 217635 is unknown.
>    https://cgit.freedesktop.org/mesa/drm/commit/?id=8b4d57e7  

That is something that will need to be tested. Given that the only Intel box I
have has a chipset with "Gen3" GPU, I cannot test any OpenCL on Intel.

>> +OPTIONS_DEFINE=	FP64 TEST
>> +FP64_DESC=	Double precision (experimental)

>I don't like this style of grouping as it doesn't scale. If you have many >options having to jump between descriptions and definitions when working on a >port with many can get tiring, see multimedia/ffmpeg.

And I don't like excess whitespace to scroll through, so it's merely a
difference of opinion. This port is not ffmpeg and it does have an unwieldy
number of options, nor is it expected to. Should there be an explosion of
options, whitespace can be added as needed.

>> +.if ${ARCH} == amd64
>> +PLIST_SUB+=	OCL20=""
>> +.else # ${ARCH} == i386
>> +PLIST_SUB+=	OCL20="@comment "
>> +.endif

>Conditionals break (mostly) declarative style of makefiles. Try the following instead

>  PLIST_SUB+=	OCL20="${ARCH:Namd64:C/.+/@comment /}"

The conditional used it immediately understandable at a glance, the other way
is not. Clarity has value greater than conciseness.

>> +	-@(cd ${TEST_WRKSRC}.utests; . ./setenv.sh; \

>Why do you need to introduce typos?

Because I'm not a robot, but an imperfect human? If you mean to ask why did I
retype it, well, I'd already bumped the port version and made some changes
(i.e. adding the OCL20 stuff), so the patch wasn't going to apply without
abandoning the progress made, copy and paste from the browser doesn't preserve
the tabs, and that particular section had excess whitespace that would need to
be removed, so retyping was the fastest solution, until I have to explain how a
typo could possibly come to be... I did not try to run the tests as I do not
expect any to succeed without the requisite hardware.

>> +-  src_data = (uint32_t*)memalign(base_address_alignment, buffer_sz);
>> +-  if(!src_data) {
>> ++  if(posix_memalign((void**)&src_data, base_address_alignment, buffer_sz)) {

>posix_memalign has greater minimal alignment than memalign or aligned_alloc in >jemalloc and glibc. And neither memalign nor posix_memalign are available on >Windows. Or do you have something against using C11/C++17 features without >passing corresponding -std= ?

The intent was the make a patch that could be upstreamed more easily.
posix_memalign is part of platforms (that doesn't include Windows). If
aligned_alloc is available on all the platforms on which posix_memalign exists,
then that is news to me. I am aware it is part of C11, but the support for
C++11 (and therefore I assume C11 as well) in GCC (still the favorite on Linux)
has been lacking every time I have tried to use it, and the last time I had to
use a Microsoft compiler it did not support C99 and left a portion of C++0x
stranded in the stdext namespace instead of std, so those are out the question.
If the C11 support in GCC is strong enough, then aligned_alloc would be fine,
though as you mention it would need the correct -std passed since GCC defaults
to gnu++98.

I do not see any more strict alignment requirements for posix_memalign; they
both require the alignment be a power of 2.
"As an example of the "supported by the implementation" requirement, POSIX
function posix_memalign accepts any alignment that is a power of two and a
multiple of sizeof(void *), and POSIX-based implementations of aligned_alloc
inherit this requirements."

>Try to avoid excessive noise (e.g. line offsets, timestamps) in patches.

I had previously been advised to use makepatch when adding and changing
patches. Is there a more strict set of criteria for when it is appropriate than
I am aware of?

> @@ -29,6 +29,8 @@
>  lib/beignet/include/ocl_vload_20.h
>  lib/beignet/include/ocl_work_group.h
>  lib/beignet/include/ocl_workitem.h
> +%%OCL20%%lib/beignet/beignet_20.bc
> +%%OCL20%%lib/beignet/beignet_20.pch
>  lib/beignet/libcl.so
>  lib/beignet/libgbe.so
>  lib/beignet/libgbeinterp.so

>Keep sorting consistent with a version where %% macros are expanded.

What is inconsistent about the sorting?
lib/beignet/b* < lib/beignet/l*

-- 
You are receiving this mail because:
You are the assignee for the bug.