Skip to content

NVTX windows include and link fixes#16831

Merged
snnn merged 2 commits into
microsoft:mainfrom
gedoensmax:nvtx_win
Aug 16, 2023
Merged

NVTX windows include and link fixes#16831
snnn merged 2 commits into
microsoft:mainfrom
gedoensmax:nvtx_win

Conversation

@gedoensmax
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

@chilo-ms and @hariharans29 for review.

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

@wschin - Can you please take a look ?

@chilo-ms
Copy link
Copy Markdown
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
Copy Markdown

Azure Pipelines successfully started running 7 pipeline(s).

@microsoft microsoft deleted a comment from azure-pipelines Bot Jul 31, 2023
@chilo-ms
Copy link
Copy Markdown
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
Copy Markdown

Azure Pipelines successfully started running 9 pipeline(s).

@gedoensmax
Copy link
Copy Markdown
Contributor Author

@chilo-ms Can we merge this ?

@chilo-ms
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor

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
Copy Markdown

Azure Pipelines successfully started running 7 pipeline(s).

@snnn snnn merged commit 7b9d1f1 into microsoft:main Aug 16, 2023
@snnn
Copy link
Copy Markdown
Contributor

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