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.

Sign up for GitHub

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

Jump to bottom

Fix DALI compilation for CUDA 11 pre 11.3 version #2925

Merged
merged 3 commits into from
May 6, 2021
Merged

Fix DALI compilation for CUDA 11 pre 11.3 version #2925

merged 3 commits into from
May 6, 2021

Conversation

JanuszL
Copy link
Contributor

@JanuszL JanuszL commented May 5, 2021

  • fixes the if condition for enabling nvJPEG HW backend with ROI
    support

Signed-off-by: Janusz Lisiecki jlisiecki@nvidia.com

Why we need this PR?

Pick one, remove the rest

  • It fixes DALI compilation for CUDA 11 pre 11.3 version

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

  • What solution was applied:
    fixes the if condition for enabling nvJPEG HW backend with ROI support
  • Affected modules and functionalities:
    nvjpeg_decoder_decoupled_api.h
  • Key points relevant for the review:
    NA
  • Validation and testing:
    CI
  • Documentation (including examples):
    NA

JIRA TASK: [NA]

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2337568]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2337569]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2337569]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2337568]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2337645]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2337646]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2337665]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2337667]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2337667]: BUILD PASSED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2337665]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2337665]: BUILD PASSED

jantonguirao
jantonguirao reviewed May 6, 2021
View reviewed changes
dali/operators/decoder/nvjpeg/nvjpeg_decoder_decoupled_api.h Outdated
@@ -74,7 +74,9 @@ class nvJPEGDecoder : public Operator<MixedBackend>, CachedDecoderImpl {
nvjpeg2k_thread_(1,
spec.GetArgument<int>("device_id"),
spec.GetArgument<bool>("affine")) {
#if NVJPEG_VER_MAJOR > 11 || (NVJPEG_VER_MAJOR == 11 && NVJPEG_VER_MINOR >= 1)
#if NVJPEG_VER_MAJOR > 11 || \
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can make this a macro so that we don't repeat it several times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@JanuszL
Fix DALI compilation for CUDA 11 pre 11.3 version
921ed0f 
- fixes the if condition for enabling nvJPEG HW backend with ROI
  support

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
@JanuszL
Review fixes
d769ea3 
Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2341397]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2341439]: BUILD STARTED

mzient
mzient reviewed May 6, 2021
View reviewed changes
dali/operators/decoder/nvjpeg/nvjpeg_decoder_decoupled_api.h Outdated
Comment on lines 42 to 45
#define IF_HW_DECODER_COMPATIBLE \
NVJPEG_VER_MAJOR > 11 || \
(NVJPEG_VER_MAJOR == 11 && (NVJPEG_VER_MINOR > 4 || \
(NVJPEG_VER_MINOR == 4 && NVJPEG_VER_PATCH >= 1)))
Copy link
Contributor

@mzient mzient May 6, 2021 •
edited

Choose a reason for hiding this comment

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

Normally when one defines such macro, the condition would be in parenthesis - otherwise there can be "interesting" results, like:
#if IF_HW_DECODER_COMPATIBLE && something would resolve to:

#if NVJPEG_VER_MAJOR > 11 || \
        (NVJPEG_VER_MAJOR == 11 && (NVJPEG_VER_MINOR > 4 || \
                                   (NVJPEG_VER_MINOR == 4 && NVJPEG_VER_PATCH >= 1))) && something

The && something only applies to the right hand side of ||.
Therefore, it should be:

Suggested change
#define IF_HW_DECODER_COMPATIBLE \
NVJPEG_VER_MAJOR > 11 || \
(NVJPEG_VER_MAJOR == 11 && (NVJPEG_VER_MINOR > 4 || \
(NVJPEG_VER_MINOR == 4 && NVJPEG_VER_PATCH >= 1)))
#define (IS_HW_DECODER_COMPATIBLE \
NVJPEG_VER_MAJOR > 11 || \
(NVJPEG_VER_MAJOR == 11 && (NVJPEG_VER_MINOR > 4 || \
(NVJPEG_VER_MINOR == 4 && NVJPEG_VER_PATCH >= 1))))

or

Suggested change
#define IF_HW_DECODER_COMPATIBLE \
NVJPEG_VER_MAJOR > 11 || \
(NVJPEG_VER_MAJOR == 11 && (NVJPEG_VER_MINOR > 4 || \
(NVJPEG_VER_MINOR == 4 && NVJPEG_VER_PATCH >= 1)))
#if NVJPEG_VER_MAJOR > 11 || \
(NVJPEG_VER_MAJOR == 11 && (NVJPEG_VER_MINOR > 4 || \
(NVJPEG_VER_MINOR == 4 && NVJPEG_VER_PATCH >= 1)))
#define IS_HW_DECODER_COMPATIBLE 1
#else
#define IS_HW_DECODER_COMPATIBLE 0
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

mzient
mzient reviewed May 6, 2021
View reviewed changes
dali/operators/decoder/nvjpeg/nvjpeg_decoder_decoupled_api.h Outdated
@@ -74,7 +78,7 @@ class nvJPEGDecoder : public Operator<MixedBackend>, CachedDecoderImpl {
nvjpeg2k_thread_(1,
spec.GetArgument<int>("device_id"),
spec.GetArgument<bool>("affine")) {
#if NVJPEG_VER_MAJOR > 11 || (NVJPEG_VER_MAJOR == 11 && NVJPEG_VER_MINOR >= 1)
#if IF_HW_DECODER_COMPATIBLE
Copy link
Contributor

Choose a reason for hiding this comment

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

This double if looks weird.

Suggested change
#if IF_HW_DECODER_COMPATIBLE
#if IS_HW_DECODER_COMPATIBLE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@JanuszL
Review fixes
b677aa4 
Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2341527]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2341528]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2341527]: BUILD PASSED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2341528]: BUILD PASSED

@JanuszL JanuszL merged commit 9d5eb48 into NVIDIA:master May 6, 2021
@JanuszL JanuszL deleted the fix_pre_11.3_compilation branch May 6, 2021 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jantonguirao jantonguirao jantonguirao approved these changes

@mzient mzient mzient approved these changes

Assignees

@jantonguirao jantonguirao

@mzient mzient

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JanuszL @dali-automaton @jantonguirao @mzient