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

Support non-tensor elements with cuda/gen backend. #1022

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

YohannDudouit
Copy link
Contributor

Try using dim == 1 for non-tensor basis to add support for non-tensor elements in the cuda/gen backend.

@jedbrown
Copy link
Member

Thanks for this.

So I'd expect this to be tested, but I see the noether-rocm test output is skipping the same number of tests from before
https://gitlab.com/libceed/libCEED/-/pipelines/582787305/test_report
image
as the new https://gitlab.com/libceed/libCEED/-/pipelines/582875782/test_report
image
Does this make sense or should fewer tests be skipped? Cc: @jeremylt

@jedbrown
Copy link
Member

Oh, probably the issue above is that this change wasn't propagated to HIP (tested on Noether). Meanwhile, lv continuous to have stability issues and we're still waiting on a CUDA GPU for Noether to arrive, at which point we should have more reliable testing.

@YohannDudouit
Copy link
Contributor Author

YohannDudouit commented Jul 14, 2022

@jedbrown I would need to work a bit more than that to make this work, it should be doable, but it is not ready (even for cuda-gen).

@jedbrown jedbrown mentioned this pull request Sep 6, 2022
4 tasks
@jedbrown
Copy link
Member

jedbrown commented Sep 6, 2022

Our CUDA CI should be reliable now. What would be needed to finish this branch, at least for cuda/gen?

@jeremylt
Copy link
Member

jeremylt commented Sep 6, 2022

I think this might be easier to get to with #1050?

@jedbrown
Copy link
Member

jedbrown commented Sep 6, 2022

Perhaps, but #1050 has possibly expansive scope so I don't know if it's feasible to finish for v0.11.

@jedbrown
Copy link
Member

How much is needed here now that #1050 has merged?

@jeremylt
Copy link
Member

I think a fair amount still needs to be done.

@YohannDudouit
Copy link
Contributor Author

So, the main issue is with the gradient, it's not just "1D" in that case, technically it's not much work, but it might be better to do something cleaner than this hack.

@jeremylt
Copy link
Member

With the refactor, we can now write and test the nontensor kernels for the shared backend and then incorporate them into the gen backend.

@YohannDudouit
Copy link
Contributor Author

@jeremylt If we follow this path we will still have to modify the gen backend, the "tensor" assumption is fairly hardcoded there a bit everywhere. I think the easiest would probably be to write a gen path for non-tensor operators (distinct from the current tensor path) using the non-tensor functions.

@jeremylt
Copy link
Member

Either way we need to write a new basis kernel. It seems simpler to test that on its own instead of testing the new kernel + the changes to the gen logic all at the same time.

@YohannDudouit
Copy link
Contributor Author

I totally agree.

@jeremylt jeremylt marked this pull request as draft March 17, 2023 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants