Add support for QPL 1.7.0 to misc codecs#1
Conversation
ElenaTyuleneva
left a comment
ElenaTyuleneva
left a comment
There was a problem hiding this comment.
I cannot provide much valuable feedback for the qpl job submission part, but there are some comments from my side regarding the codec's integration in general.
A comment that is common to all changes: not all steps from CONTRIBUTING.md are done in the scope of these changes(e.g. CHANGELOG, Readme), but I suppose it will be done closer to the final version of the code.
| #endif | ||
|
|
||
| #ifndef BENCH_REMOVE_QPL | ||
| char* lzbench_qpl_init(size_t insize, size_t, size_t); |
There was a problem hiding this comment.
As I understood from the function's definition, the last parameter(blocks_number) is actually used, so it may be useful to align the parameters naming in the definition and declaration just for the verbosity in both places.
| return outsize; | ||
| } | ||
|
|
||
| #endif // DBENCH_REMOVE_QPL |
There was a problem hiding this comment.
| #endif // DBENCH_REMOVE_QPL | |
| #endif // BENCH_REMOVE_QPL |
|
|
||
| qpl_compression_levels qpl_level = (codec_options->level == 3) ? qpl_high_level : qpl_default_level; | ||
|
|
||
| compress(ctx, in, insize, out, &outsize, qpl_level, false); |
There was a problem hiding this comment.
Probably, this function should have a more qpl-specific name. To prevent overlapping with other functions in the benchmark that may be named this way. The same question is applicable for decompress()
|
|
||
| misc/libqpl/qpl-wrappers/qpl_helper.o: misc/libqpl/qpl-wrappers/qpl_helper.cpp | ||
| @$(MKDIR) $(dir $@) | ||
| $(CXX) $(CFLAGS) -c $< -o $@ |
There was a problem hiding this comment.
Should we pass $(CXXFLAGS) here?
| endif | ||
| endif # ifneq "$(LIBCUDART)" | ||
|
|
||
| HAS_CMAKE_NASM = $(shell command -v cmake >/dev/null 2>&1 && command -v nasm >/dev/null 2>&1 && echo 1 || echo 0) |
There was a problem hiding this comment.
Double-checking: if the version of nasm or CMake is lower than required by qpl, it's expected that this error will be handled by qpl's build system?
| $(CC) $(CFLAGS) -mavx $< -c -o $@ | ||
|
|
||
| misc/libqpl/qpl/release/lib64/libqpl.a: | ||
| $(MKDIR) misc/libqpl/qpl/build && cmake -S misc/libqpl/qpl -B misc/libqpl/qpl/build -DCMAKE_BUILD_TYPE=Release -DQPL_BUILD_EXAMPLES=OFF -DQPL_BUILD_TESTS=OFF -DCMAKE_INSTALL_PREFIX=misc/libqpl/qpl/release && make -C misc/libqpl/qpl/build install |
There was a problem hiding this comment.
We potentially need to check if qpl's built-in flags don't contradict the statement from the main Readme:
"...This method provides the advantage of compiling all compressors with the same compiler and optimizations...."
or can just highlight that to lzbench owners and see their feedback.
2 participants
No description provided.