V1.1 Tools Interface: incremental, action-based

[DavidPoliakoff]

Alternative to #3799

Rather than have a different set of callbacks, in which a tool can tell Kokkos what to do (the 3799 approach), this is much more incremental, but probably just as featureful. Basically, at init, Kokkos requests that a tool enumerate its requirements (global fencing), and gives the tool a list of actions (function pointers) it can take (like fence Kokkos)

Still after init, we request that a tool tell us whether it requires the current "global" fence behavior in a new callback (kokkosp_request_responses). This callback accepts a pointer to a struct in which the tool can declare settings. Right now it only has one setting (requires_global_fencing, default true), but this leaves us room to expand. One problem with expanding it, though, is that a tool might be from a newer Kokkos than an application, so I enumerate how many responses are supported by Kokkos. I'll also try to be diligent about updating KOKKOSP_INTERFACE_VERSION, but that's a bit of a crapshoot, so this is an extra guard rail. Speaking of guard rails, if a tool doesn't implement this function, the result is that no settings are overwritten. Since the settings default to current behavior, we guarantee (check me on this) backwards compatibility.

As to how the tool fences Kokkos, we have another callback (kokkosp_transmit_actions), that takes in a ToolActions struct containing a set of function pointers. At this point, all we have is a "fence" function that takes in a device ID. At the moment, that device ID is ignored, but long term we might change the invoked function from a global Kokkos::fence() to something more selective. ToolActions is implemented the same as ToolResponses, it has padding and reports the number of implemented callbacks, so tools can reason about what is supported.

Personally, at this point I favor this approach over that of 3799, it's powerful, but a very small change.

edit: additionally, replaced the "function_pointer" typedef in the C_Interface.h file with Kokkos_Tools_functionPointer, to avoid name collisions and make the naming scheme a bit more consistent

[DavidPoliakoff]

Quick note: I've not yet actually removed the hard-coded fences. We can do that if this kind of design looks good, this is just a sketch of what tools need to do

[DavidPoliakoff]

Retest this please

[DavidPoliakoff]

Just for yuks I've reimplemented the Apollo autotuning tool for this interface, and it does work, kinda cool to see. So the tuning use case is supported by this

[DavidPoliakoff]

For an example of a tool that has integrated this, see LLNL/apollo#12 , where fences are enabled up til convergence, at which point they're removed

[crtrott]

Generally I like this much better, in particular since it will actually work :-)

Besides the comment left inline, two naming things:

• How about renaming ToolActions into ToolInvokedActions or so. Make it more clear that these are things where the tool calls stuff provided by Kokkos.
• How about ToolResponses into ToolSettings: Responses sounds a bit like its something we query the tool over and over for. Settings is more clear that its a one time thing, we ask the tool to figure out how to behave on our side.

[DavidPoliakoff]

My plan is to implement Christian's changes once I get a "this works for me" from Kevin and/or Bill. Procuring such a statement would likely require a little bit of testing things, which I'd be more than happy to do

[khuck]

I enthusiastically endorse this change... I am trying to profile the WDM XGC app, and it uses Kokkos. If I set KOKKOS_PROFILE_LIBRARY, the application grinds to a halt with all the fencing. If I don't set that variable and ignore Kokkos events, it runs smooth as silk. But then Kokkos is a blind spot in the profile.

[DavidPoliakoff]

@crtrott : made those changes. If Kevin's happy, I'm happy

[crtrott]

Almost there :-) Try to make for 3.4

[nliber]

Instead of just a comment, how about replacing BooleanConstant (which is a description of the type and not its purpose) with something like enum class RequiresGlobalFencing : bool { Yes, No } . Given that this is inlined, the check will likely always be optimized away (or use a two different functions differentiated by a tag or use RequiresGlobalFencing as a non-type template parameter that we specify explicitly if we are really concerned about this).

I'm also wondering if Callback and RequiresGlobalFencing should be switched, as callback and its args should be together.

[DavidPoliakoff]

I've addressed the comments from the call. I think I'll leave it at these changes, and improve the implementation details more in a future PR