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.

Sign up for GitHub

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

Jump to bottom

Const for CeedQFunctionGetSourcePath #1488

Merged
merged 7 commits into from
Feb 23, 2024
Merged

Const for CeedQFunctionGetSourcePath #1488

merged 7 commits into from
Feb 23, 2024

Conversation

jeremylt
Copy link
Member

Missed a const

@jeremylt jeremylt self-assigned this Feb 23, 2024
@jedbrown
Copy link
Member

Please hold off on this; I have a few more.

@jeremylt
Copy link
Member Author

There are a few that have to be non-const. Some of them are intended for the user to free the string after they are done

@jeremylt
Copy link
Member Author

jeremylt commented Feb 23, 2024 •
edited

CeedQFunctionLoadSourceToBuffer needs to be non-const, but CeedQFunctionGetFields and CeedOperatorGetFields probably should be swapped to make the returned lists const (though that probably gets messy in the Rust interface) as well probably CeedCompositeOperatorGetSubList needs to be const. CeedElemRestrictionGetStrides, CeedElemRestrictionGetLLayout, and CeedElemRestrictionGetELayout copy values to a user owned array, so no const needed.

@jedbrown
internal: const for source paths
f8d308f 
Add const to CeedQFunctionLoadSourceToBuffer output argument. If we
had Result, this would look like

  let const source_buffer = CeedQFunctionLoadSourceToBuffer()?;

The caller is free to cast in the const when passing the output variable
if they want this semantic:

  let mut source_buffer = CeedQFunctionLoadSourceToBuffer()?;
@jedbrown
Copy link
Member

I recall reading Linus writing on this point some time ago, advocating using const in structs even when the object needs to be freed. Note that due to how const works, CeedFree(&ptr) works fine even if ptr is const`.

Have a look at my commit message for my thoughts on CeedQFunctionLoadSourceToBuffer.

I'm unsure about my change to backends/sycl-ref/ceed-sycl-ref-qfunction-load.sycl.cpp, which would need to be checked using a sycl compiler (and I wouldn't cry if we just drop it from this commit until we have SYCL CI).

jeremylt
jeremylt commented Feb 23, 2024
View reviewed changes
interface/ceed-jit-tools.c Outdated Show resolved Hide resolved
@jeremylt
Copy link
Member Author

Ahh, I didn't realize you could free a const like that. It doesn't always line up with my expectations

@jeremylt
Copy link
Member Author

Ok, looks like CI is happy. I'm fine merging with the SYCL change since it should be fine and mirrors the changes in CUDA/HIP that pass

@jedbrown
Copy link
Member

Nope, wait a minute. I reproduced and will fix things on Noether.

@jedbrown
Copy link
Member

Should be good now once the pipeline passes. To build, I use:

$ . /opt/intel/oneapi/setvars.sh
$ make CC=icx CXX=icpx SYCL_DIR=/opt/intel/oneapi/compiler/2023.2.0/linux

We could make a new job in .gitlab-ci.yml that only builds.

@jeremylt
Copy link
Member Author

Small issue in the CUDA/HIP backends that's fixed now

     SYCLCXX build/backends/sycl/ceed-sycl-common.o
     SYCLCXX build/backends/sycl/ceed-sycl-compile.o
     SYCLCXX build/backends/sycl/online_compiler.o
     SYCLCXX build/backends/sycl-ref/ceed-sycl-ref-basis.o
     SYCLCXX build/backends/sycl-ref/ceed-sycl-ref-operator.o
     SYCLCXX build/backends/sycl-ref/ceed-sycl-ref-qfunction-load.o
     SYCLCXX build/backends/sycl-ref/ceed-sycl-ref-qfunction.o
     SYCLCXX build/backends/sycl-ref/ceed-sycl-ref-qfunctioncontext.o
     SYCLCXX build/backends/sycl-ref/ceed-sycl-ref.o
     SYCLCXX build/backends/sycl-ref/ceed-sycl-restriction.o
     SYCLCXX build/backends/sycl-ref/ceed-sycl-vector.o
     SYCLCXX build/backends/sycl-shared/ceed-sycl-shared-basis.o
     SYCLCXX build/backends/sycl-shared/ceed-sycl-shared.o
     SYCLCXX build/backends/sycl-gen/ceed-sycl-gen-operator-build.o
     SYCLCXX build/backends/sycl-gen/ceed-sycl-gen-operator.o
     SYCLCXX build/backends/sycl-gen/ceed-sycl-gen-qfunction.o
     SYCLCXX build/backends/sycl-gen/ceed-sycl-gen.o

and manually checked SYCL is still fine with my update

@jeremylt
Copy link
Member Author

CI is wrapping up and will pass. I think we just need an approval + merge

@jeremylt jeremylt merged commit 1f70653 into main Feb 23, 2024
27 of 28 checks passed
@jeremylt jeremylt deleted the jeremy/const-path branch February 23, 2024 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedbrown jedbrown jedbrown approved these changes

Assignees

@jeremylt jeremylt

Labels
1-In Review minor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jeremylt @jedbrown