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

Work around spurious "missing return statement" warnings for NVCC and Intel compilers #1102

Merged
merged 3 commits into from
Jun 7, 2024

Conversation

JaredCrean2
Copy link
Contributor

This is a small change to avoid a compiler warning on both Intel and Nvidia compilers. The warning is incorrect, but it is muddying up compile output and the dashboard results for the Sierra Toolkit.

The warning is that (sometimes) the compiler thinks:

double foo()
{
  if constexpr (...)
  {
    return 42;
  } else
  {
    return 0;
  }
}  // warning here about lack of return statement in non-void function

is missing a return statement.

Copy link
Contributor

@aprokop aprokop left a comment

Choose a reason for hiding this comment

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

@JaredCrean2 Thank you for looking into it! Sorry about the issue, we will fix the nvcc warnings in the near future (#272).

In the mean time, I tested the compiler version on godbolt, and every NVCC version prior to 11.5 and Intel (icc) prior to 2021.7 are susceptible.

To be a bit kinder to ourselves in the future, could you please try out the following patch:

--- a/src/details/ArborX_DetailsLegacy.hpp
+++ b/src/details/ArborX_DetailsLegacy.hpp
@@ -52,6 +52,12 @@ public:
       expand(bounding_volume, Access::get(_primitives, i));
       return value_type{bounding_volume, (index_type)i};
     }
+#if (defined(KOKKOS_COMPILER_NVCC) && (KOKKOS_COMPILER_NVCC < 1150)) || \
+    (defined(KOKKOS_COMPILER_INTEL) && (KOKKOS_COMPILER_INTEL <= 2021))
+    // FIXME_NVCC, FIXME_INTEL: workaround for spurios "missing return
+    // statement at end of non-void function" warning
+    return value_type{};
+#endif
   }

   KOKKOS_FUNCTION

and see if it fixes the issue? I would prefer this way to clearly indicate when we can clean this up in the future, as well as to not affect the code for any other compilers.

Note: all Intel compilers 2021.* have 2021 as version.

@aprokop
Copy link
Contributor

aprokop commented Jun 6, 2024

Kokkos does it slightly different: https://github.com/kokkos/kokkos/blob/63f05204d0eaeec83e25f87eeb8025a188683f92/algorithms/src/std_algorithms/impl/Kokkos_UniqueCopy.hpp#L178-L181, by relying on __builtin_unreachable(). However, that doesn't work with NVCC prior to 11.3.

@aprokop aprokop changed the title work around incorrect compiler warnings Work around spurious "missing return statement" warnings for NVCC and Intel compilers Jun 6, 2024
@JaredCrean2
Copy link
Contributor Author

The patch does fix the warnings. For the intel compiler, the details are:

[jccrean@cee-build032 code]$ mpicxx --version
icpc (ICC) 2021.3.0 20210609

And the warning is:

/ceeGPFS/fgs/jccrean/code/TPLs_src/ArborX/include/details/ArborX_DetailsLegacy.hpp(58): warning #1011: missing return statement at end of non-void function "ArborX::Details::LegacyValues<Primitives, BoundingVolume>::operator() [with Primitives=stk::search::impl::ViewWrapperForArborXTraits<Kokkos::View<const std::pair<stk::search::Point<double>, <unnamed>::Id> *, Kokkos::HostSpace, Kokkos::MemoryTraits<1U>>>, BoundingVolume=ArborX::Box]"
    }
    ^
         

Although I can't get godbolt to reproduce it with a minimal example. The Intel classic compiler is deprecated in favor of the new Clang-based compiler, so may ifdefing on __INTEL_COMPILER is good enough (the Clang-base compiler has __INTEL_LLVM_COMPILER defined to distinguish itself from the classic compiler)

@aprokop
Copy link
Contributor

aprokop commented Jun 6, 2024

Although I can't get godbolt to reproduce it with a minimal example. The Intel classic compiler is deprecated in favor of the new Clang-based compiler, so may ifdefing on __INTEL_COMPILER is good enough (the Clang-base compiler has __INTEL_LLVM_COMPILER defined to distinguish itself from the classic compiler)

Here's the reproducer for Intel (classic, icc). The newer versions of Intel (icx LLVM-based) seem to be ok. Kokkos provides KOKKOS_COMPILER_INTEL for classic, and KOKKOS_COMPILER_INTEL_LLVM for legacy.

I'm not sure if you noticed I edited my patch to include Intel.

@JaredCrean2
Copy link
Contributor Author

Ah, I was missing the -Wall. I did check your patch against the Intel compiler in the Sierra Toolkit (STK) code and it works.

Do you want me to update this PR with the new patch, or do you want to make a new PR?

@aprokop
Copy link
Contributor

aprokop commented Jun 6, 2024

Do you want me to update this PR with the new patch, or do you want to make a new PR?

Please update this PR.

@JaredCrean2
Copy link
Contributor Author

Done. Thanks for looking at this so quickly @aprokop

@aprokop
Copy link
Contributor

aprokop commented Jun 6, 2024

Done. Thanks for looking at this so quickly @aprokop

Needs formatting fix. I can do that if you prefer.

@JaredCrean2
Copy link
Contributor Author

I added a commit after running clang-format. Feel free to make any additional changes if needed.

@aprokop
Copy link
Contributor

aprokop commented Jun 7, 2024

GCC failed for unrelated reason (cc1plus killed). Merging.

@aprokop aprokop merged commit 0bc3cab into arborx:master Jun 7, 2024
0 of 2 checks passed
@aprokop aprokop mentioned this pull request Jun 10, 2024
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