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

[ Meson ] BCQTensor dependency is added for Android build #2823

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

EunjuYang
Copy link
Contributor

  • This commit add BiQGEMM path to nntrainer_inc_abs to support Android build with enable-biqgemm option.
  • The path for BiQGEMM corresponds to the one in the top meson.build

Self evaluation:

Build test: [X]Passed [ ]Failed [ ]Skipped
Run test: [X]Passed [ ]Failed [ ]Skipped

@@ -11,6 +11,17 @@ nntrainer_inc_abs = [
meson.source_root() / 'api' / 'ccapi' / 'include'
]

# The code below feeds BiQGEMM absolute path to android.mk
# The BiQGEMM absolute path is included only when biqgemm is enabled.
if get_option('enable-biqgemm')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure, this won't affect tools/package_android.sh, isn't it?

Copy link
Contributor Author

@EunjuYang EunjuYang Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This won't affect to the current tools/package_android.sh. This aims for the case where meson build -Dplatform=android -Denable-biqgemm=true.

Copy link
Contributor

@djeong20 djeong20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
One minor suggestion: How about adding it to the tensor/meson.build for consistency?

if get_option('enable-biqgemm')
tensor_headers += 'bcq_tensor.h'
tensor_sources += 'bcq_tensor.cpp'
nntrainer_inc += biqgemm_inc
endif

@EunjuYang EunjuYang force-pushed the meson/bcqtensor_android branch from 949d373 to 78067df Compare December 11, 2024 01:07
- This commit add BiQGEMM path to nntrainer_inc_abs to support Android
  build with enable-biqgemm option.

**Self evaluation:**

Build test: [X]Passed [ ]Failed [ ]Skipped
Run test: [X]Passed [ ]Failed [ ]Skipped

Signed-off-by: Eunju Yang <[email protected]>
@EunjuYang EunjuYang force-pushed the meson/bcqtensor_android branch from 78067df to 0854dde Compare December 11, 2024 01:09
@EunjuYang
Copy link
Contributor Author

EunjuYang commented Dec 11, 2024

LGTM! One minor suggestion: How about adding it to the tensor/meson.build for consistency?

if get_option('enable-biqgemm')
tensor_headers += 'bcq_tensor.h'
tensor_sources += 'bcq_tensor.cpp'
nntrainer_inc += biqgemm_inc
endif

Following the suggestion from @djeong20, I moved the code to nntrainer/tensor/meson.build.
Since the code to check BiQGEMM folder's existence is completed at the precedent meson file, the duplicate check code is removed; simply the one line is added.

nntrainer_inc_abs += meson.source_root() / '..' / 'BiQGEMM'

Copy link
Collaborator

@jijoongmoon jijoongmoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jijoongmoon jijoongmoon merged commit bff8025 into nnstreamer:main Dec 11, 2024
18 checks passed
@myungjoo
Copy link
Member

myungjoo commented Dec 11, 2024

meson.source_root() / '..' / 'BiQGEMM'

DO NOT HARDCODE EXTERNAL PATHS IN MESON SCRIPT!
It is completely UN-portable.

If pkgconfig or cmake is not ready with BiQGEMM,
get such information via meson_options and specify such "default paths" at meson_options.

@myungjoo
Copy link
Member

And..

if get_option('enable-biqgemm')
  # check if BiQGEMM directory exist. otherwise, throw an error
  fs = import('fs')
  if fs.is_dir('../BiQGEMM')
    extra_defines += '-DENABLE_BIQGEMM=1'
    biqgemm_inc = include_directories('../BiQGEMM')
  else
    error ('BiQGEMM cannot be enabled without BiQGEMM library.')
  endif
endif

is completely non-sense. Other developers (e.g., nntrainer users in other departments) won't understand what's going on here.

You should try

  1. get biqgemm info from pkgconfig/cmake.
  2. (if 1 fails) try to load it from common path (/usr/include, /usr/include/biqgemm)
  3. (if 2 fails) try to load it from user-defined path (meson_option)
  4. (if 3 fails) try to load it from such hardcoded path. but specify such external hardcoded path at meson_options.txt, not in this script.

@EunjuYang
Copy link
Contributor Author

@myungjoo, Thank you for the detailed comment and guidance :) I will make additional PR to make it feasible following your suggestion. Sincerely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants