warnings from GCC 10.1 in reduce.hpp for boolean reductions

[Dan Bonachea]

test/collectives.cpp is triggering a warning from GCC 10.1 in reduce.hpp for boolean reductions on at least one platform ( EX-ppc64el-smp-gcc-cupc2c, which simulates GCC on Summit).

The meat of the warning:

/home/phargrov/upcnightly/EX-ppc64el-smp-gcc-cupc2c/runtime/work/dbg/upcxx-inst/bin/upcxx -o collectives-par /home/phargrov/upcnightly/EX-ppc64el-smp-gcc-cupc2c/runtime/work/dbg/upcxx/test/collectives.cpp -Wall -Wno-unused
In file included from /home/phargrov/upcnightly/EX-ppc64el-smp-gcc-cupc2c/runtime/work/dbg/upcxx/test/collectives.cpp:4:
/home/phargrov/upcnightly/EX-ppc64el-smp-gcc-cupc2c/runtime/work/dbg/upcxx-inst/upcxx.debug.gasnet_par.smp/include/upcxx/reduce.hpp: In instantiation of 'T upcxx::detail::opfn_mul::operator()(T, Tb&&) const [with T = bool; Tb = bool&]':
/home/phargrov/upcnightly/EX-ppc64el-smp-gcc-cupc2c/runtime/work/dbg/upcxx-inst/upcxx.debug.gasnet_par.smp/include/upcxx/reduce.hpp:120:25:   required from 'static void upcxx::detail::reduce_op_slow_id<Op, T>::op_vecfn(const void*, void*, std::size_t, const void*) [with Op = upcxx::detail::op_wrap<upcxx::detail::opfn_mul, true>; T = bool; std::size_t = long unsigned int]'
/home/phargrov/upcnightly/EX-ppc64el-smp-gcc-cupc2c/runtime/work/dbg/upcxx-inst/upcxx.debug.gasnet_par.smp/include/upcxx/reduce.hpp:294:50:   required from 'typename upcxx::detail::completions_returner<upcxx::detail::event_is_here, upcxx::detail::reduce_scalar_event_values<T>, Cxs>::return_t upcxx::detail::reduce_one_or_all_trivial(T1&&, BinaryOp, upcxx::intrank_t, const upcxx::team&, Cxs) [with T1 = bool; BinaryOp = upcxx::detail::op_wrap<upcxx::detail::opfn_mul, true>; Cxs = upcxx::completions<upcxx::future_cx<upcxx::operation_cx_event, upcxx::progress_level::user> >; T = bool; typename upcxx::detail::completions_returner<upcxx::detail::event_is_here, upcxx::detail::reduce_scalar_event_values<T>, Cxs>::return_t = upcxx::future1<upcxx::detail::future_kind_shref<upcxx::detail::future_header_ops_general>, bool>; upcxx::intrank_t = int]'
/home/phargrov/upcnightly/EX-ppc64el-smp-gcc-cupc2c/runtime/work/dbg/upcxx-inst/upcxx.debug.gasnet_par.smp/include/upcxx/reduce.hpp:518:64:   required from 'typename upcxx::detail::completions_returner<upcxx::detail::event_is_here, upcxx::detail::reduce_scalar_event_values<T>, Cxs>::return_t upcxx::reduce_all(T1, BinaryOp, const upcxx::team&, Cxs) [with T1 = bool; BinaryOp = upcxx::detail::op_wrap<upcxx::detail::opfn_mul, true>; Cxs = upcxx::completions<upcxx::future_cx<upcxx::operation_cx_event, upcxx::progress_level::user> >; T = bool; typename upcxx::detail::completions_returner<upcxx::detail::event_is_here, upcxx::detail::reduce_scalar_event_values<T>, Cxs>::return_t = upcxx::future1<upcxx::detail::future_kind_shref<upcxx::detail::future_header_ops_general>, bool>]'
/home/phargrov/upcnightly/EX-ppc64el-smp-gcc-cupc2c/runtime/work/dbg/upcxx/test/collectives.cpp:88:76:   required from here
/home/phargrov/upcnightly/EX-ppc64el-smp-gcc-cupc2c/runtime/work/dbg/upcxx-inst/upcxx.debug.gasnet_par.smp/include/upcxx/reduce.hpp:49:18: warning: '*' in boolean context, suggest '&&' instead [-Wint-in-bool-context]
   40 |           a tok##= std::forward<Tb>(b);\
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
......
   49 |   UPCXX_INFIX_OP(*, mul, false)
/home/phargrov/upcnightly/EX-ppc64el-smp-gcc-cupc2c/runtime/work/dbg/upcxx-inst/upcxx.debug.gasnet_par.smp/include/upcxx/reduce.hpp:40:13: note: in definition of macro 'UPCXX_INFIX_OP'
   40 |           a tok##= std::forward<Tb>(b);\
      |             ^~~

Basically -Wint-in-bool-context (enabled by -Wall) wants us to explicitly use && for and-ing bool types, rather than (confusingly) relying on * to have that same effect via integral promotion and [conv.bool]. Of course this arises here because we are using macro/template expansion that applies to all integral types.

I believe the warning is harmless in this particular case, but would rather not globally suppress the warning (especially because this occurs in a public header, so that would impact user code). Ideally we can find a localized workaround in the header, either by special-casing bool or adding scoped warning suppression.

[Amir Kamil]

This is fixable with an operator overload for bool:

diff --git a/src/reduce.hpp b/src/reduce.hpp
index de7be5be..a4e05438 100644
--- a/src/reduce.hpp
+++ b/src/reduce.hpp
@@ -31,7 +31,7 @@ namespace upcxx {
   /* `detail::opfn_[add|...]` is the function object which actually implements
    * `operator()` and is used as the value for `OpFn` in `op_wrap`.
    */
-  #define UPCXX_INFIX_OP(tok, name, integral_only1) \
+  #define UPCXX_INFIX_OP(tok, tok_bool, name, integral_only1) \
     namespace detail {\
       struct opfn_##name {\
         static constexpr bool integral_only = integral_only1;\
@@ -40,16 +40,21 @@ namespace upcxx {
           a tok##= std::forward<Tb>(b);\
           return static_cast<T&&>(a);\
         }\
+        template<typename Tb>\
+        bool operator()(bool a, Tb &&b) const {\
+          a tok_bool##= std::forward<Tb>(b);\
+          return static_cast<bool&&>(a);\
+        }\
       };\
     }\
     constexpr detail::op_wrap<detail::opfn_##name, /*fast_demanded=*/false> op_##name = {};\
     constexpr detail::op_wrap<detail::opfn_##name, /*fast_demanded=*/true> op_fast_##name = {};

-  UPCXX_INFIX_OP(+, add, false)
-  UPCXX_INFIX_OP(*, mul, false)
-  UPCXX_INFIX_OP(&, bit_and, true)
-  UPCXX_INFIX_OP(|, bit_or, true)
-  UPCXX_INFIX_OP(^, bit_xor, true)
+  UPCXX_INFIX_OP(+, |, add, false)
+  UPCXX_INFIX_OP(*, &, mul, false)
+  UPCXX_INFIX_OP(&, &, bit_and, true)
+  UPCXX_INFIX_OP(|, |, bit_or, true)
+  UPCXX_INFIX_OP(^, ^, bit_xor, true)
   #undef UPCXX_INFIX_OP

   namespace detail {

However, the real culprit is the test code in collectives.cpp:

  auto and2_done = upcxx::reduce_all(bool(sum1 & 1), upcxx::op_fast_mul, tm);

We are purposely testing bool with op_fast_mul. I don’t think we have a right to expect that to work without a warning.

[Dan Bonachea]

We are purposely testing bool with op_fast_mul. I don’t think we have a right to expect that to work without a warning.

The spec explicitly permits this usage in 12.2-22:

If T is bool, then op_fast_add and op_fast_max perform the same operation as op_fast_bit_or, and op_fast_mul and op_fast_min perform the same op- eration as op_fast_bit_and.

Whether it should generate a warning or not is debatable, but I don't like to see warnings in library code for behaviors we explicitly permit.

Your fix looks right to me.

[Amir Kamil]

Add bool overloads for reduction operators. Fixes #376.

→ <<cset 35bb0ac3cb9e>>