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

NVTX windows include and link fixes #16831

Merged
merged 2 commits into from
Aug 16, 2023
Merged

NVTX windows include and link fixes #16831

merged 2 commits into from
Aug 16, 2023

Conversation

gedoensmax
Copy link
Contributor

Description

For windows headers are not duplicated to the normal cuda include. For linux they are:

(base) maximilianm@maximilianm-dt-linux:~$ ls /usr/local/cuda/include/nvtx3 | grep nvTool
nvToolsExt.h
nvToolsExtCuda.h
nvToolsExtCudaRt.h
nvToolsExtOpenCL.h
nvToolsExtSync.h
(base) maximilianm@maximilianm-dt-linux:~$ ls /usr/local/cuda/include | grep nvTool
nvToolsExt.h
nvToolsExtCuda.h
nvToolsExtCudaRt.h
nvToolsExtOpenCL.h
nvToolsExtSync.h

Is the preference via those added defines or should the include just be changed to be nvtx3/ ?

Also there is no library linking needed on Windows and the library is not even present.

@gedoensmax
Copy link
Contributor Author

@chilo-ms and @hariharans29 for review.

@hariharans29 hariharans29 requested a review from wschin July 31, 2023 21:00
@hariharans29
Copy link
Member

@wschin - Can you please take a look ?

@chilo-ms
Copy link
Contributor

/azp run Windows GPU TensorRT CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, onnxruntime-python-checks-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, Linux QNN CI Pipeline, Windows ARM64 QNN CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 7 pipeline(s).

@microsoft microsoft deleted a comment from azure-pipelines bot Jul 31, 2023
@chilo-ms
Copy link
Contributor

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux Nuphar CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, Windows CPU CI Pipeline, Windows GPU CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@gedoensmax
Copy link
Contributor Author

@chilo-ms Can we merge this ?

@chilo-ms
Copy link
Contributor

or windows headers are not duplicated to the no

I've asked @wschin for helping review it.
Also, I saw ORT Web CI failures, could you merge main and try it again?

@gedoensmax
Copy link
Contributor Author

/* Arithmetic FP16 operations only supported on arch >= 5.3 */
#if !defined(__CUDA_ARCH__) || (__CUDA_ARCH__ >= 530) || defined(_NVHPC_CUDA)

These lines are removed from CUDA 12.2 which is why I also just added another commit fixing that in common.cuh. Hope that's fine otherwise just let me know.

@chilo-ms
Copy link
Contributor

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux Nuphar CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, Windows CPU CI Pipeline, Windows GPU CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@chilo-ms chilo-ms requested a review from snnn August 15, 2023 22:05
@snnn
Copy link
Member

snnn commented Aug 16, 2023

/azp run Linux QNN CI Pipeline, Windows ARM64 QNN CI Pipeline, Windows GPU TensorRT CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed

@azure-pipelines
Copy link

Azure Pipelines successfully started running 7 pipeline(s).

@snnn snnn merged commit 7b9d1f1 into microsoft:main Aug 16, 2023
66 checks passed
@snnn
Copy link
Member

snnn commented Aug 16, 2023

I tried it locally. It works very good.

kleiti pushed a commit to kleiti/onnxruntime that referenced this pull request Mar 22, 2024
### Description

For windows headers are not duplicated to the normal cuda include. For
linux they are:
```
(base) maximilianm@maximilianm-dt-linux:~$ ls /usr/local/cuda/include/nvtx3 | grep nvTool
nvToolsExt.h
nvToolsExtCuda.h
nvToolsExtCudaRt.h
nvToolsExtOpenCL.h
nvToolsExtSync.h
(base) maximilianm@maximilianm-dt-linux:~$ ls /usr/local/cuda/include | grep nvTool
nvToolsExt.h
nvToolsExtCuda.h
nvToolsExtCudaRt.h
nvToolsExtOpenCL.h
nvToolsExtSync.h
```
Is the preference via those added defines or should the include just be
changed to be `nvtx3/` ?

Also there is no library linking needed on Windows and the library is
not even present.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants