Skip to content

Fix a warning in pool.cc#8168

Merged
snnn merged 1 commit intomasterfrom
snnn/patch-4
Jun 28, 2021
Merged

Fix a warning in pool.cc#8168
snnn merged 1 commit intomasterfrom
snnn/patch-4

Conversation

@snnn
Copy link
Copy Markdown
Contributor

@snnn snnn commented Jun 27, 2021

Description:

The warning is:
"Potential comparison of a constant with another constant. at D:\a_work\1\s\onnxruntime\core\providers\cuda\nn\pool.cc@167,21".

It was found by VS static code analyzer in our CUDA EP.

Motivation and Context

  • Why is this change required? What problem does it solve?

This change is related to #8041. Because we are in C++17 now, we should use "if constexpr" when applicable.

  • If it fixes an open issue, please link to the issue here.

@snnn snnn marked this pull request as ready for review June 27, 2021 20:15
@snnn snnn requested a review from a team as a code owner June 27, 2021 20:15

cudnnPoolingMode_t mode = CUDNN_POOLING_MAX;
if (PoolType::type == onnxruntime::PoolType::kAveragePool) {
if constexpr (PoolType::type == onnxruntime::PoolType::kAveragePool) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not ORT_IF_CONSTEXPR ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's a cc file. ORT_IF_CONSTEXPR is for header files that are also used in *.cu files. This file is compiled by GCC which can support C++17 well. The main limitation comes from nvcc, which compiles *.cu files.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But, we can use ORT_IF_CONSTEXPR in all the places. Was that your question?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes :)

@snnn snnn merged commit 9b75be3 into master Jun 28, 2021
@snnn snnn deleted the snnn/patch-4 branch June 28, 2021 14:58
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.

3 participants