Disposition of several *_KOKKOS_* compiler directives

[jtostie]

There are a few compiler directives within the LCM problems and evaluators that seem to be related to KOKKOS and/or CUDA development. Specifically,

ALBANY_KOKKOS_UNDER_DEVELOPMENT
KOKKOS_HAVE_CUDA
PHX_KOKKOS_DEVICE_TYPE_CUDA

What is the status of this effort? The existence of these in the main LCM evaluators impedes readability and generally causes confusion. This has become evident to me in doing code walkthroughs recently. I would like to inquire as to whether or not they can be safely removed.

Thoughts, objections, agreement?

[bartgol]

The first one I think is "ok" to have it. I think it is used to switch between plain foor loops or Kokkos-based loops. As long as it is there only during a conversion phase, it can be ok.

The other two are more annoying. Not only, as you said, they make the code less readable, but they make the code backend-specific. This goes against (one of) the purpose of Kokkos, which is to create a single code base for mulitple architectures. The only place where such macros make sense is in a main header file where some other compile-time quantities/types are introduced, such as an AlbanyExecSpace type or a compile-time constant default team size for the execution policies, which may be chosen differently depending on the device type. Such cases are ok, and sometimes necessary. But having, say, an evaluator doing different things based on the device type is different, and borderline wrong.

My 2 cents.

[ibaned]

@lxmota

[lxmota]

I completely agree with @bartgol that having device specific code is plainly wrong, and it's against the original purpose of Kokkos.

I say that we eliminate all of these directives from LCM. As @jtostie says, they are confusing and prevent people from understanding the code. This is one of the reasons I get some much flak about LCM.

For ALBANY_KOKKOS_UNDER_DEVELOPMENT, if the Kokkos loop works, let's get rid of the for loop. Otherwise flag it as TODO and remove the directive.

The other two CUDA directives are wrong in my opinion. We cannot burden our users/developers to learn the specifics of CUDA. If there is any specific CUDA code, remove it.

If there are no objections to this, I'll go ahead and do the above when I find such code in LCM. Feel free to do the same if you run across it.

[mperego]

I agree with @lxmota and @bartgol. I have removed all the ALBANY_KOKKOS_UNDER_DEVELOPMENT #ifdef guards in FELIX a while ago, getting rid of the "non Kokkos" code. So I'm totally OK with you doing the same with LCM.

I don't know why Cuda specific code was added to LCM, but my understanding is that there is not an active push for having LCM run on GPUs at the moment. If that's the case, I do not see reasons for "polluting" LCM code with ifdef guards that are not needed.
FELIX is going to be a test bed for running Albany on GPUs, and I suspect there will be many changes in next years on the way the model evaluators work, so I expect that the current Cuda specific code in LCM will be obsolete soon anyway.

[ikalash]

@mperego is correct: LCM does not work with CUDA currently. More than a year ago, when @jewatkins and I were working on getting as much of Albany working with CUDA and setting up the test harness on Ride, we could that LCM would not compile with CUDA so it is not on in my nightly test harness.

Just to play devil's advocate, I think most people would agree non-ALBANY_KOKKOS_UNDER_DEVELOPMENT code is much more readable/understandable, so likely new LCM users will too. That would be an argument for keeping the original code rather than the Kokkos version; but I'm fine with keeping the ALBANY_KOKKOS_UNDER_DEVELOPMENT code if it works at least w/o GPUs in LCM, and getting rid of the original code, towards moving in the direction of Kokkos / performance portability.

[bartgol]

@ikalash Yes, I agree that non-kokkos version of evaluateFields is more readable, since it is more "standard" c++. However, if kept along side kokko code, then it should be kept for all classes (at least PHAL basic evaluators), and for classes with several partial template specialization (such as PHAL interpolations) it could add up to a lot of code in a single file. I would argue that very long files are also harder to read...

[lxmota]

This last point is something that perhaps we should discuss at the Albany meeting. I'll make a note of it and bring it up then.

[jtostie]

I would vote to keep the non-kokkos versions in the LCM code base, partly because of readability issues, but also because of comments like Kinematics_Def.hpp line 200 which indicates to me it may not even work.

If there is any LCM related development or deliverables over the next 2-3 years that requires Kokkos, I am not aware of it. Personally I would be happy to wait until the interface stabilizes some more given what @mperego mentioned above with regard to FELIX being a develop GPU testbed.

[lxmota]

@jtostie Let's do it this way then. Keep the non-Kokkos code and remove CUDA specific code.