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

Drop JiT Guards in Most QF Source #1540

Merged
merged 11 commits into from Mar 28, 2024
Merged

Drop JiT Guards in Most QF Source #1540

merged 11 commits into from Mar 28, 2024

Conversation

jeremylt
Copy link
Member

Follow-up on #1534 and #1537, dropping QF source header guards where able

@jeremylt
Copy link
Member Author

Fluids is the only one left - need to check for multiply included files and still guard those

Working on parallel changes in Ratel

@jrwrigh
Copy link
Collaborator

jrwrigh commented Mar 27, 2024

Is there any harm in adding #pragma once to all of them? It should work for CPUs and the GPU backends should handle the case fine, right?

@jeremylt
Copy link
Member Author

#pragma once won't do anything for the GPU backends as all the included files are put into one long string buffer, but I think I have handled that with #1532. My plan is to first drop all guards that I can without using #pragma once and then see how many files that leaves. Then I'll see if #pragma once works with what's left.

@jeremylt
Copy link
Member Author

Do you know if SYCL JiT handles #pragma once in a source file fine?

@jeremylt jeremylt force-pushed the jeremy/less-jit-guards branch 2 times, most recently from 7d6be0b to 2e430ef Compare March 27, 2024 20:02
@jeremylt
Copy link
Member Author

Turns out, ROCm doesn't like JiTing #pragma once so we can't do that.

@jrwrigh
Copy link
Collaborator

jrwrigh commented Mar 27, 2024

RIP, not sure about SYCL JIT, but I guess if ROCM doesn't then we probably shouldn't for any of the others.

Or (if we really wanted to) we could have CeedLoadSourceToInitializedBuffer_Private remove any #pragma once and rely on it to only allow one header for the JITed backends.

@jeremylt
Copy link
Member Author

jeremylt commented Mar 27, 2024

I'm hesitant to hide too much from the users. I think the way to go for now is to ask users to use #ifndef style guards under the umbrella of "mutually supported C99 + ROCm + CUDA" syntax. This PR drops 767 lines we don't need anymore, so I think we take the win

@jedbrown
Copy link
Member

Is there any #pragma once in code that could be concatenated into a string, or is that always staying in headers that we just #include?

@jeremylt
Copy link
Member Author

In this branch pragma once is not in any file that we turn into a string.

@jrwrigh
Copy link
Collaborator

jrwrigh commented Mar 27, 2024

I'm hesitant to hide too much from the users.

That's valid, but also we're already implementing what the #pragma once directive does. So it should be transparent to the user at all times, regardless. We basically just wrote a really simple preprocessor for your JIT string processing.

But it's 6-half-dozen-or-the-other to me.

@jeremylt
Copy link
Member Author

The easy part is enabling the pragma for QFunctions. Backend JiT source is harder, but I have an idea

@jeremylt
Copy link
Member Author

Ok, "turned on" #pragma once for QFunction source by stripping it out.

The way that Basis source is built means this won't work without another backend only interface, but I know what I have to expose for that to work.

Copy link
Collaborator

@jrwrigh jrwrigh left a comment

Choose a reason for hiding this comment

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

Few comments, overall looks good.

examples/fluids/qfunctions/riemann_solver.h Outdated Show resolved Hide resolved
examples/fluids/qfunctions/stabilization.h Outdated Show resolved Hide resolved
tests/t406-qfunction.h Show resolved Hide resolved
@jeremylt jeremylt force-pushed the jeremy/less-jit-guards branch 2 times, most recently from f239508 to c5b0d5b Compare March 28, 2024 15:40
@jeremylt jeremylt merged commit 509d4af into main Mar 28, 2024
28 checks passed
@jeremylt jeremylt deleted the jeremy/less-jit-guards branch March 28, 2024 16:30
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.

None yet

3 participants