Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JiT include update #1696

Merged
merged 8 commits into from
Oct 21, 2024
Merged

JiT include update #1696

merged 8 commits into from
Oct 21, 2024

Conversation

jeremylt
Copy link
Member

@jeremylt jeremylt commented Oct 16, 2024

Update JiT to -I all the JiT directories set on the Ceed.

Note: Most of the changes are repeated ripple effects from the core changes it ceed-[cuda,hip]-compile.h.

  1. Allow searching of dirs set with CeedAddJitSourceRoot for #include <foo.h>

  2. Add CEED_RUNNING_JIT_PASS to guard files that cannot be included for JiT (math.h or std*.h)

  3. Prefer ceed/types.h over ceed.h in QF source

  4. Redirect ceed.h to ceed/types.h over ceed/ceed.h when CEED_RUNNING_JIT_PASS is set

  5. Use include statements instead of loading string buffers for JiT

Ok, here's a rough prototype of an idea @jedbrown. This allows #include <foo.h> (stripping out #include <std*.h>) and would add all JiT dirs as -I/include/this/dir flags to the CUDA/HIP JiT compilers.

Very rough and not rigorously tested but seems to work locally with hacky manual testing.

@jeremylt jeremylt self-assigned this Oct 16, 2024
interface/ceed-jit-tools.c Outdated Show resolved Hide resolved
Copy link
Member

@jedbrown jedbrown left a comment

Choose a reason for hiding this comment

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

Probably this means we could delete code related to actually finding those headers and reading them. At a glance, this looks good and presumably reduces the volume of output when using CEED_DEBUG=1.

@jrwrigh
Copy link
Collaborator

jrwrigh commented Oct 16, 2024

Probably this means we could delete code related to actually finding those headers and reading them.

I'm guessing the OpenCL online compiler has similar functionality, but we'll need to check that for the SYCL backends.

@jeremylt jeremylt force-pushed the jeremy/jit-include branch 7 times, most recently from 8b077cc to ae8472b Compare October 16, 2024 22:19
@jeremylt
Copy link
Member Author

Need to run full suite tomorrow, but seems fine with Ratel (should be)

@jeremylt jeremylt force-pushed the jeremy/jit-include branch 3 times, most recently from e979080 to b5030a2 Compare October 17, 2024 15:42
@jeremylt
Copy link
Member Author

Ratel is happy. Noether looks happy.

I say we merge this and do further improvements as follow-up.

@NamjaeChoi
Copy link

Hello, I checked out your branch and tested it, but the error persists:
/data/choin/moose/libCEED/interface/ceed-jit-tools.c:138 in CeedLoadSourceToInitializedBuffer(): Couldn't open source file: /data/choin/projects/moose/framework/build/header_symlinks/Kokkos_Core.hpp
This file is in another directory and I provided that directory to CeedAddJitSourceRoot. ~/header_symlinks is where the QFunction source is located.

@jeremylt
Copy link
Member Author

This remains really difficult for me to fully understand without the actual code generating the issue

@jeremylt
Copy link
Member Author

Guessing here, if you are using a local include #include "Kokkos_Core.hpp" instead of #include <Kokkos_Core.hpp>, then as we talked about in the other thread, that local include is still being interpreted as a relative path from the QFunction source file during code generation.

@NamjaeChoi
Copy link

Ah so #include "header.h" still searches for the local directory only?

@jeremylt
Copy link
Member Author

Yes, at this time we're still interpreting #include "foo.h" as a local file

@NamjaeChoi
Copy link

Changing to #include <header.h> seems to work. Let me try further. Thank you!

@NamjaeChoi
Copy link

Do you plan to change the behavior of #include "header.h" as well?

@jeremylt
Copy link
Member Author

jeremylt commented Oct 17, 2024

Not in this PR, no.

See: #1695 (reply in thread)

@jeremylt jeremylt force-pushed the jeremy/jit-include branch 2 times, most recently from 05c4fc1 to fb6f31a Compare October 18, 2024 15:32
@jeremylt jeremylt merged commit 1dc8b1e into main Oct 21, 2024
26 of 28 checks passed
@jeremylt jeremylt deleted the jeremy/jit-include branch October 21, 2024 16:14
jrwrigh added a commit that referenced this pull request Oct 22, 2024
jrwrigh added a commit that referenced this pull request Oct 23, 2024
* doc: Update release notes based on #1696

Co-authored-by: Jeremy L Thompson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants