(Trilinos) build warning in atomic_assign, with Kokkos::complex

[mhoemmen]

When building Trilinos, GCC 4.7.2 reports a build warning in Kokkos::atomic_assign, line 301 of kokkos/core/src/impl/Kokkos_Atomic_Exchange.hpp:

http://testing.sandia.gov/cdash/viewBuildError.php?type=1&buildid=2318891

warning: implicit dereference will not access object of type ‘volatile Kokkos::complex’ in statement [enabled by default]

I'm not sure if this is harmless, or a problem with Kokkos::complex, but I just wanted to report it.

[hcedwar]

Harmless but annoying warning I have seen before. I will put harmless & annoying no-op in in the function to suppress the warning.

From: Mark Hoemmen <notifications@github.commailto:notifications@github.com>
Reply-To: kokkos/kokkos <reply@reply.github.commailto:reply@reply.github.com>
Date: Thursday, January 28, 2016 at 7:06 AM
To: kokkos/kokkos <kokkos@noreply.github.commailto:kokkos@noreply.github.com>
Subject: [EXTERNAL] kokkos build warning in atomic_assign, with Kokkos::complex (#177)

When building Trilinos, GCC 4.7.2 reports a build warning in Kokkos::atomic_assign, line 301 of kokkos/core/src/impl/Kokkos_Atomic_Exchange.hpp:

http://testing.sandia.gov/cdash/viewBuildError.php?type=1&buildid=2318891

warning: implicit dereference will not access object of type 'volatile Kokkos::complex' in statement [enabled by default]

I'm not sure if this is harmless, or a problem with Kokkos::complex, but I just wanted to report it.

Reply to this email directly or view it on GitHubhttps://github.com//issues/177.

[crtrott]

Yeah its kind of hard to suppress it. Its effectively an unused return value from fetch and add.

[mhoemmen]

Harmless but annoying warning I have seen before.

I saw it was in an atomic_* function and left it alone like a sane person should ;-)

If the no-op doesn't work, we can try the GCC pragmas (push / pop suppression of a particular warning). They don't work for all the warnings, though. Anyway, thanks for taking a look!

[mhoemmen]

@bartlettroscoe says:

One option [to disable this Kokkos warning in Trilinos] is to pass in the include dirs for Kokkos using -isystem <dir> instead of -I <dir>. That should turn off warnings coming from Kokkos.

[mhoemmen]

@hcedwar I'm getting pushback from Trilinos leaders about this one; would you consider reopening it?

[crtrott]

Did the no-op not work?

[bartlettroscoe]

One option [to disable this Kokkos warning in Trilinos] is to pass in the include dirs for Kokkos using -isystem <dir> instead of -I <dir>. That should turn off warnings coming from Kokkos.

Just to be clear, this is currently not supported by TriBITS. I was just saying that we could consider that as part of:

• EPIC: Merge TriBITS concepts and implementation of Packages and TPLs TriBITSPub/TriBITS#63

That would require a change in the way that TriBITS manages include dirs from upstream package (see this todo).

[mhoemmen]

@crtrott Unfortunately not. Compilers seem to have learned not to accept the "cast-to-void" idiom.

I haven't had any customers complain about it. The only reason this came up is because Mike Heroux mentioned that test builds with warnings as errors weren't passing, and wondered what was going on. It would be nice to get those builds through Tpetra, but again, no customers are complaining. (They must not enable the warning in question.)

@bartlettroscoe Thanks for the clarification! This could also be useful to applications that use Kokkos through its Makefile build system.

[ibaned]

It could be suppressed if the Kokkos::complex volatile assignment operator returned void instead of a reference.

https://github.com/kokkos/kokkos/blob/master/core/src/Kokkos_Complex.hpp#L127

The difference is that an assignment statement could not be used as a value, for example
a = b = c
These are fairly rare uses, especially if only the volatile assignment operator is changed. I've done this for one of my classes that needed volatile assignment to be used in Kokkos:

https://github.com/ibaned/omega_h2/blob/master/src/int128.hpp#L22

[hcedwar]

I have argued a similar point in the ISO/C++ committee over atomic overloads for '=' and '+='.
I have lost the argument due to do-not-break-backward-compatibility trump card.
However, I believe returning void from the volatile (and atomic) overloads is the right thing to do.
Will re-open for this discussion.

[hcedwar]

The semantic concern is race condition in the update-then-read. Should the read value be the updated value of that operation or should should it reflect a potentially intervening update for the most-up-to-the-instant value? When returning void there is no concern since update and read are always explicitly separate operations.

[mhoemmen]

@hcedwar Users would normally only exercise the volatile overload of Kokkos::complex in the join() of a parallel_{reduce, scan}. If they try to do a = b = c and that overload returns void, they will get a build error. We can add a comment to Kokkos::complex explaining this. I'm with @ibaned ; I get grumpy whenever I see code like a = b = c. If making that overload return void fixes the problem, I'm all for it.

[crtrott]

Yeah I think that is ok. Also std::complex doesn't have the volatile overloads at all, so making them behave differently shouldn't hurt.

[mhoemmen]

Hm, line 296 of the implementation of Kokkos::atomic_fetch_add (for >64-bit types) does the following:

const T tmp = *dest = return_val + val;

This means operator= must return something other than void. Can I change this line? Would that have horrible consequences for weird memory models like POWER's?

[nmhamster]

@mhoemmen POWER8 is the same as X86 here, the operations are not executed concurrently, they are in C/C++ program order. If you were to break these into two statements it would be the "same" code.

[mhoemmen]

@nmhamster Can I get rid of const T tmp = here? That's the part that causes the warning.

If it needs to be there, somebody please explain to me why, and I'll put the comment explaining it right above the line of code myself.

[ibaned]

Some git blame suggests that it was added by @crtrott in commit 2fd9fb0 to silence this exact warning anyways.

[nmhamster]

@mhoemmen if the result is thrown away the compiler will just not generate the code anyway (unless T is volatile). I'd advocate for using a GCC pragma unused attribute if we have to silence warnings.

[mhoemmen]

.../Trilinos/packages/kokkos/core/src/impl/Kokkos_Atomic_Exchange.hpp:310:3: warning: conversion to void will not access object of type volatile Kokkos::complex<double>

   (void)( *dest = val );

The GCC pragmas to silence warnings (pragma GCC diagnostic push, ignored "-Wall", pop) don't work.

[mhoemmen]

Thanks @nmhamster for encouragement and help! I think I've figured this out.

This will require a backwards-incompatible change to both Kokkos::complex and Kokkos::pair. In particular, multiple assignments in a single statement (like z = y = x;) for these two types will no longer work if the middle variable is volatile. This is acceptable to me because (a) multiple assignments in a single statement are annoying to read, and (b) explicitly volatile variables in the Kokkos context typically only occur in a join() of a parallel reduce or scan, so this interface change should not affect much code.

This also appears to be the right thing to do with respect to GCC's
treatment of volatiles:

https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Volatiles.html

@hcedwar and @crtrott , how do you feel about this change?

[hcedwar]

For volatile qualified types it is the right thing to do.

[mhoemmen]

For volatile qualified types it is the right thing to do.

I'll put it in develop then. I can test Trilinos (doing so right now) but will have to wait to test e.g., LAMMPS until the next develop -> master integration.

[ibaned]

Nice ! just one thing: can we remove the instances of:

const T tmp = *dest;
(void) tmp;

? I think they were there in multiple assignment form to silence the unused reference warning, which is no longer necessary. They don't seem to do anything at all otherwise

[mhoemmen]

@ibaned Do those cause warnings for you? I really don't want to remove them, because this is tricky code. (For example, const T tmp = *dest; has the side effect of invoking a memory load of the volatile output, which may have something to do with correctness on some platforms.) I'll wait for @crtrott or @nmhamster to comment.

[ibaned]

No warnings here, just an uneducated guess on my part. (conversely, if I'm right, we could avoid an unnecessary load in performance-critical code).

[hcedwar]

Yep - put there to suppress warning. A decent optimizer would eliminate the code, but would be better not to have it in the first place.

[nmhamster]

@mhoemmen or @ibaned can you give me co-ordinates for a specific piece of code I can look at. This appears to be rather nasty hack to get ride of the compile warnings but I want to check it won't affect correctness. In general loading a value (volatile or not) shouldn't affect correctness if the result is discarded but we should check.

[nmhamster]

Not sure a decent optimizer would get rid of the load if T has volatile type. This is required in code which performs things like memory-mapped I/O (like drivers etc).

[ibaned]

@nmhamster here are (I think all) the links:

kokkos/core/src/impl/Kokkos_Atomic_Exchange.hpp

Line 241 in 9f23b5f

const T tmp = *dest;

kokkos/core/src/impl/Kokkos_Atomic_Compare_Exchange_Strong.hpp

Line 231 in 9f23b5f

const T tmp = *dest;

kokkos/core/src/impl/Kokkos_Atomic_Fetch_Add.hpp

Line 306 in 9f23b5f

const T tmp = *dest;