Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vector feature #71

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

shabanovd
Copy link
Collaborator

No description provided.

@shabanovd
Copy link
Collaborator Author

@freemo Here vector feature "proof of concept".

Right now, VectorTest generate valid OpenCL code but fail to execute because of !!!!!!! clCreateBuffer failed invalid buffer size. If I understand it right, something should be changed at JNI code to create correct buffer for vector array. @freemo, will you able to help with it?

@codecov-io
Copy link

codecov-io commented Oct 23, 2017

Codecov Report

Merging #71 into master will decrease coverage by 0.17%.
The diff coverage is 26.66%.

@@             Coverage Diff              @@
##             master      #71      +/-   ##
============================================
- Coverage     46.93%   46.75%   -0.18%     
- Complexity      849      856       +7     
============================================
  Files            57       58       +1     
  Lines          9386     9460      +74     
  Branches       1527     1547      +20     
============================================
+ Hits           4405     4423      +18     
- Misses         4538     4589      +51     
- Partials        443      448       +5

@freemo
Copy link
Member

freemo commented Oct 23, 2017 via email

@freemo freemo added the enhancement Adds new functionality label Oct 23, 2017
@freemo freemo added this to the 1.5.0 milestone Oct 23, 2017
@freemo
Copy link
Member

freemo commented Oct 23, 2017

@shabanovd ok so I tracked down the change (I think) to this line:

https://github.com/Syncleus/aparapi-native/blob/b7473d1f8a00284b56dfe7ec1185f70e9a1eedb6/src/cpp/invoke/OpenCLMem.cpp#L5

The issue is figuring out how to change it for an array of primitives and vectors at the same time.

@freemo
Copy link
Member

freemo commented Oct 23, 2017

@shabanovd
Copy link
Collaborator Author

@freemo think that it end up in adjusting OpenCLMem::getPrimitiveSizeInBytes by something like:

...
jsize vectorFactor = 1;
if (argisset(argBits, VECTOR2) {
    vectorFactor = 2;
} else if (argisset(argBits, VECTOR3) {
    vectorFactor = 3;
} else ...
return(sizeInBytes * vectorFactor);

Does that make sense?

@freemo
Copy link
Member

freemo commented Oct 23, 2017

@shabanovd yea that makes sense. I will give it a try and see if it passes tests. You have all the unit tests in your PR I can test against i assume?

@shabanovd
Copy link
Collaborator Author

@freemo yes, use this test

@freemo
Copy link
Member

freemo commented Oct 23, 2017

@shabanovd wonderful thanks. I'll edit the native code this evening and run the tests. If it works ill check it in. BTW if you feel compelled to do it yourself we also have aparapi-vagrant which brings up a vagrant environment with AMD APP SDK installed and all the build tools so you can built linux x63 and x32 binaries. The native project also should compile as-is under OSX just fine... windows is still a bit of a PITA to compile though.

@freemo
Copy link
Member

freemo commented Oct 24, 2017

@shabanovd with your suggested edits to aparapi-native the compilation fails with the following results:

ibtool: compile:  g++ -DPACKAGE_NAME=\"libaparapi\" -DPACKAGE_TARNAME=\"libaparapi\" -DPACKAGE_VERSION=\"1.2.1\" "-DPACKAGE_STRING=\"libaparapi 1.2.1\"" -DPACKAGE_BUGREPORT=\"[email protected]\" -DPACKAGE_URL=\"\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DPACKAGE=\"libaparapi\" -DVERSION=\"1.2.1\" -I. -Iinclude -I/opt/AMDAPP/SDK/include -Isrc/cpp -Isrc/cpp/runKernel -Isrc/cpp/invoke -I/usr/lib/jvm/java-8-openjdk/include -I/usr/lib/jvm/java-8-openjdk/include/linux -I/usr/lib/jvm/java-8-openjdk-amd64/include -I/usr/lib/jvm/java-8-openjdk-amd64/include/linux -I/usr/lib/jvm/java-8-oracle/include -I/usr/lib/jvm/java-8-oracle/include/linux -I/System/Library/Frameworks/JavaVM.framework/Versions/Current/Headers/ -DCL_USE_DEPRECATED_OPENCL_1_1_APIS -g -O2 -MT src/cpp/invoke/libaparapi_la-OpenCLArgDescriptor.lo -MD -MP -MF src/cpp/invoke/.deps/libaparapi_la-OpenCLArgDescriptor.Tpo -c src/cpp/invoke/OpenCLArgDescriptor.cpp  -fno-common -DPIC -o src/cpp/invoke/.libs/libaparapi_la-OpenCLArgDescriptor.o
mv -f src/cpp/invoke/.deps/libaparapi_la-OpenCLArgDescriptor.Tpo src/cpp/invoke/.deps/libaparapi_la-OpenCLArgDescriptor.Plo
/bin/sh ./libtool  --tag=CXX   --mode=compile g++ -DPACKAGE_NAME=\"libaparapi\" -DPACKAGE_TARNAME=\"libaparapi\" -DPACKAGE_VERSION=\"1.2.1\" -DPACKAGE_STRING=\"libaparapi\ 1.2.1\" -DPACKAGE_BUGREPORT=\"[email protected]\" -DPACKAGE_URL=\"\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DPACKAGE=\"libaparapi\" -DVERSION=\"1.2.1\" -I.  -Iinclude -I/opt/AMDAPP/SDK/include -Isrc/cpp -Isrc/cpp/runKernel -Isrc/cpp/invoke -I/usr/lib/jvm/java-8-openjdk/include -I/usr/lib/jvm/java-8-openjdk/include/linux -I/usr/lib/jvm/java-8-openjdk-amd64/include -I/usr/lib/jvm/java-8-openjdk-amd64/include/linux -I/usr/lib/jvm/java-8-oracle/include -I/usr/lib/jvm/java-8-oracle/include/linux -I/System/Library/Frameworks/JavaVM.framework/Versions/Current/Headers/ -DCL_USE_DEPRECATED_OPENCL_1_1_APIS   -g -O2 -MT src/cpp/invoke/libaparapi_la-OpenCLMem.lo -MD -MP -MF src/cpp/invoke/.deps/libaparapi_la-OpenCLMem.Tpo -c -o src/cpp/invoke/libaparapi_la-OpenCLMem.lo `test -f 'src/cpp/invoke/OpenCLMem.cpp' || echo './'`src/cpp/invoke/OpenCLMem.cpp
libtool: compile:  g++ -DPACKAGE_NAME=\"libaparapi\" -DPACKAGE_TARNAME=\"libaparapi\" -DPACKAGE_VERSION=\"1.2.1\" "-DPACKAGE_STRING=\"libaparapi 1.2.1\"" -DPACKAGE_BUGREPORT=\"[email protected]\" -DPACKAGE_URL=\"\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DPACKAGE=\"libaparapi\" -DVERSION=\"1.2.1\" -I. -Iinclude -I/opt/AMDAPP/SDK/include -Isrc/cpp -Isrc/cpp/runKernel -Isrc/cpp/invoke -I/usr/lib/jvm/java-8-openjdk/include -I/usr/lib/jvm/java-8-openjdk/include/linux -I/usr/lib/jvm/java-8-openjdk-amd64/include -I/usr/lib/jvm/java-8-openjdk-amd64/include/linux -I/usr/lib/jvm/java-8-oracle/include -I/usr/lib/jvm/java-8-oracle/include/linux -I/System/Library/Frameworks/JavaVM.framework/Versions/Current/Headers/ -DCL_USE_DEPRECATED_OPENCL_1_1_APIS -g -O2 -MT src/cpp/invoke/libaparapi_la-OpenCLMem.lo -MD -MP -MF src/cpp/invoke/.deps/libaparapi_la-OpenCLMem.Tpo -c src/cpp/invoke/OpenCLMem.cpp  -fno-common -DPIC -o src/cpp/invoke/.libs/libaparapi_la-OpenCLMem.o
src/cpp/invoke/OpenCLMem.cpp:54:8: error: use of undeclared identifier 'com_aparapi_internal_opencl_OpenCLArgDescriptor_ARG_VECTOR2_BIT'
   if (argisset(argBits, VECTOR2) {
       ^
src/cpp/invoke/JavaArgs.h:8:42: note: expanded from macro 'argisset'
#define argisset(bits, token) (((bits) & arg(token)) == arg(token))
                                         ^
src/cpp/invoke/JavaArgs.h:7:21: note: expanded from macro 'arg'
#define arg(token) (com_aparapi_internal_opencl_OpenCLArgDescriptor_ARG_##token##_BIT)
                    ^
<scratch space>:50:1: note: expanded from here
com_aparapi_internal_opencl_OpenCLArgDescriptor_ARG_VECTOR2_BIT
^
src/cpp/invoke/OpenCLMem.cpp:54:8: error: use of undeclared identifier 'com_aparapi_internal_opencl_OpenCLArgDescriptor_ARG_VECTOR2_BIT'
src/cpp/invoke/JavaArgs.h:8:57: note: expanded from macro 'argisset'
#define argisset(bits, token) (((bits) & arg(token)) == arg(token))
                                                        ^
src/cpp/invoke/JavaArgs.h:7:21: note: expanded from macro 'arg'
#define arg(token) (com_aparapi_internal_opencl_OpenCLArgDescriptor_ARG_##token##_BIT)
                    ^
<scratch space>:50:1: note: expanded from here
com_aparapi_internal_opencl_OpenCLArgDescriptor_ARG_VECTOR2_BIT
^
2 errors generated.
make: *** [src/cpp/invoke/libaparapi_la-OpenCLMem.lo] Error 1

It is really late and I'm too tired to investigate this right now (I can help more tomorrow) but it would appear you simple forgot to add your support for Vectors in this class: https://github.com/Syncleus/aparapi/blob/master/src/main/java/com/aparapi/internal/opencl/OpenCLArgDescriptor.java

@shabanovd
Copy link
Collaborator Author

@freemo I was able to solve buffer size on java side. There is already way to build buffer from "complex" object and restore it after OpenCL execution. That is good news because of no needs to change aparapi-native.

Vector feature on final step now!

@freemo
Copy link
Member

freemo commented Oct 24, 2017

@shabanovd Awesome, are all the tests passing now? Is the pr ready to accept/review?

@shabanovd
Copy link
Collaborator Author

@freemo not yet. It's just prove of concept. Now I need cover all vector operations, write tests and etc. It'll take this and next weeks.

@freemo
Copy link
Member

freemo commented Oct 24, 2017

@shabanovd sounds good. Let me know where I can help.

@freemo
Copy link
Member

freemo commented Feb 11, 2018

@shabanovd Any progress on this? I'm working on a new release soon and curious if i can help get this PR in on time or not.

@shabanovd
Copy link
Collaborator Author

@freemo do you have any deadline for release? ... I'll need to get back into it before I can recall what TODOs till it done.

@freemo
Copy link
Member

freemo commented Feb 15, 2018

@shabanovd No no specific deadline. Just want to make sure it stays active and synced with master.

@freemo freemo modified the milestones: 1.5.0, 1.6.0 Mar 27, 2018
@freemo freemo modified the milestones: 1.6.0, 1.7.0 Apr 15, 2018
@freemo freemo removed this from the 1.7.0 milestone Apr 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adds new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants