Skip to content

Conversation

@NamjaeChoi
Copy link
Contributor

@NamjaeChoi NamjaeChoi commented Oct 1, 2025

Refs #30655.

Functors are not dispatched but plugged inside the user's parallel code, so I can't think of a way to achieve static polymorphism here. I think I will have to introduce virtual functions minimally in the wrapper class, but any idea to avoid it would be welcome, if you have any. Of course, if the user specifies the exact object type instead of using the abstract type, they will be able to evaluate functors statically.

Concerns are performance and SYCL, but regarding SYCL, Intel is adding an extension to allow virtual functions on GPU. We may be able to leverage it when the time comes to support SYCL. I don't think we are interested in supporting SYCL for backends other than Intel GPU.

@NamjaeChoi
Copy link
Contributor Author

@lindsayad My next thing to look into would be this. I will start from functions. Do you have any thoughts on this?

@lindsayad
Copy link
Member

If we use virtuals, should we consider just adapting the current functor system?

@NamjaeChoi
Copy link
Contributor Author

The problem with that is we can't construct MOOSE objects directly on device. If an object that has virtual methods is constructed on host, the vtable will be populated with host function pointers. What I'm trying to do here is to define a thin wrapper that has virtual methods and is light enough to be built on device, and let the wrapper call the wrapped functor methods statically.

Copy link
Member

@lindsayad lindsayad left a comment

Choose a reason for hiding this comment

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

I think this looks ok to me

@NamjaeChoi NamjaeChoi changed the title WIP: Kokkos Functor System Kokkos Function System and Functor Scratch Oct 23, 2025
@NamjaeChoi NamjaeChoi changed the title Kokkos Function System and Functor Scratch Kokkos Function System Oct 23, 2025
@NamjaeChoi
Copy link
Contributor Author

Still working... just want to run test suite once

@NamjaeChoi NamjaeChoi force-pushed the kokkos_functor branch 3 times, most recently from c723295 to 68df514 Compare October 24, 2025 00:13
@moosebuild
Copy link
Contributor

moosebuild commented Oct 24, 2025

Job Documentation, step Docs: sync website on 2d163bd wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

moosebuild commented Oct 24, 2025

Job Coverage, step Generate coverage on 2d163bd wanted to post the following:

Framework coverage

0cc44f #31653 2d163b
Total Total +/- New
Rate 86.02% 85.98% -0.04% 100.00%
Hits 124557 124799 +242 493
Misses 20246 20354 +108 0

Diff coverage report

Full coverage report

Modules coverage

Coverage did not change

Full coverage reports

Reports

This comment will be updated on new commits.

@NamjaeChoi NamjaeChoi force-pushed the kokkos_functor branch 3 times, most recently from 2d9d24b to 977be17 Compare October 24, 2025 19:19
@lindsayad
Copy link
Member

going to unsubscribe for now. Mention me when you want more input or it's ready for review

@NamjaeChoi
Copy link
Contributor Author

NamjaeChoi commented Oct 24, 2025

@lindsayad I might have to hold this. Virtual functions on device seem to require RDC to be fully functional.

@NamjaeChoi
Copy link
Contributor Author

The code is currently in a state where CUDA will work when we have RDC.

@NamjaeChoi
Copy link
Contributor Author

I don't want to indefinitely hold this PR for RDC. I will temporarily disable using virtual dispatch for GPU.

@NamjaeChoi NamjaeChoi force-pushed the kokkos_functor branch 3 times, most recently from f9d7ff6 to 1b25b33 Compare November 2, 2025 03:11
@NamjaeChoi NamjaeChoi marked this pull request as ready for review November 3, 2025 20:32
@NamjaeChoi
Copy link
Contributor Author

NamjaeChoi commented Nov 3, 2025

@lindsayad You can review this. I temporarily blocked the code path that requires RDC with a runtime error as the ETA of it is unknown.

@NamjaeChoi NamjaeChoi force-pushed the kokkos_functor branch 2 times, most recently from 679c2de to 1b668cb Compare November 3, 2025 20:49
Comment on lines 9 to 15
!listing test/tests/kokkos/functions/piecewise_constant/kokkos_piecewise_constant.i start=[func_x_y] end=[] include-end=true

!listing test/tests/kokkos/functions/piecewise_constant/kokkos_piecewise_constant.i start=[func_xy_data] end=[] include-end=true

!listing test/tests/kokkos/functions/piecewise_constant/kokkos_piecewise_constant.i start=[func_csv] end=[] include-end=true

!listing test/tests/kokkos/functions/piecewise_constant/kokkos_piecewise_constant.i start=[func_json] end=[] include-end=true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
!listing test/tests/kokkos/functions/piecewise_constant/kokkos_piecewise_constant.i start=[func_x_y] end=[] include-end=true
!listing test/tests/kokkos/functions/piecewise_constant/kokkos_piecewise_constant.i start=[func_xy_data] end=[] include-end=true
!listing test/tests/kokkos/functions/piecewise_constant/kokkos_piecewise_constant.i start=[func_csv] end=[] include-end=true
!listing test/tests/kokkos/functions/piecewise_constant/kokkos_piecewise_constant.i start=[func_json] end=[] include-end=true
!listing test/tests/kokkos/functions/piecewise_constant/kokkos_piecewise_constant.i block=KokkosFunctions

I've grown suspicious that we're missing some doc hooks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

Did you just not want to show the top-level block before? I was worried that there was a documentation issue if you used the syntax I suggested. But the fact that you made the changes I suggested makes me not worry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to I show all the four possible data formats, but your change also serves the same purpose.

Comment on lines 33 to 34
if (fp.Parse(_name, vars) == -1) // -1 for success
mooseWarning("Function name '" + _name + "' could evaluate as a ParsedFunction");
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this warning. The actual name of the function in the input file is parsable? This warning should add more context if it remains. Like "name parses to a function... we don't use the name itself as a function; hopefully that was not your intent"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the issue linked to this warning is #7659. Do you want to improve the message?

Copy link
Member

Choose a reason for hiding this comment

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

I would change it to an error in your new action. And then yea I think we could improve the message string in both the old action (which remains a warning in case someone somewhere is relying on the behavior) and your new action to describe the possible conflict outlined in #7659

type = 'Exodiff'
input = 'kokkos_constant_function.i'
exodiff = 'kokkos_constant_function_out.e'
requirement = 'The Kokkos function system shall include a constant function.'
Copy link
Member

Choose a reason for hiding this comment

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

Mention ability to retrieve abstract type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm the context is a bit weird.

Copy link
Member

Choose a reason for hiding this comment

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

I would just like it mentioned somewhere given that you have a distinct requirement regarding the concrete type

@moosebuild
Copy link
Contributor

Job Precheck, step Clang format on a3b2b2f wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/docs/PRs/31653/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 0cc44f880140671a55e87b96a292d17d48977833

@NamjaeChoi NamjaeChoi force-pushed the kokkos_functor branch 2 times, most recently from d03ea8b to 81360be Compare November 4, 2025 21:29
@NamjaeChoi NamjaeChoi force-pushed the kokkos_functor branch 2 times, most recently from 37a7a98 to 0e59f51 Compare November 4, 2025 23:33
@moosebuild
Copy link
Contributor

Job Test, step Results summary on 2d163bd wanted to post the following:

Framework test summary

Compared against 0cc44f8 in job civet.inl.gov/job/3354977.

Removed tests

Added tests

Test Time (s)
kokkos/functions/piecewise_constant.piecewise_constant/json 1.46
kokkos/functions/piecewise_constant.piecewise_constant/xy_data 1.34
kokkos/functions/piecewise_constant.piecewise_constant/csv 1.31
kokkos/functions/piecewise_constant.piecewise_constant/x_y 1.27
kokkos/functions/concrete_type.invalid_type 1.26
kokkos/functions/constant_function.test 1.16
kokkos/functions/concrete_type.concrete_type 1.16
kokkos/functions/default_function.test 1.10

Run time changes

Modules test summary

Compared against 0cc44f8 in job civet.inl.gov/job/3354977.

Removed tests

Added tests

Run time changes

Test Base (s) Head (s) +/-
stochastic_tools/test:samplers/mcmc.mcmc_sampling/pmcmc_base_bounds 2.93 4.76 +62.16%
stochastic_tools/test:web_server_control.stochastic_control/batch_reset 3.32 5.20 +56.61%
stochastic_tools/test:samplers/mcmc.mcmc_sampling/pmcmc_base 2.89 4.43 +53.63%

@NamjaeChoi
Copy link
Contributor Author

Any other comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

3 participants