Skip to content

Conduit Device Support - #1358

Draft
JustinPrivitera wants to merge 228 commits into
developfrom
task/JustinPrivitera/05_01_24/device_support
Draft

Conduit Device Support#1358
JustinPrivitera wants to merge 228 commits into
developfrom
task/JustinPrivitera/05_01_24/device_support

Conversation

@JustinPrivitera

@JustinPrivitera JustinPrivitera commented Jan 24, 2025

Copy link
Copy Markdown
Member

Resolves #1151

See #1614 for ongoing development feature-tracking

@siramok

This comment was marked as outdated.

@JustinPrivitera

This comment was marked as outdated.

Comment on lines +474 to +477
#if defined(_WIN32) && defined(CONDUIT_WINDOWS_DLL_EXPORTS) && \
!defined(CONDUIT_EXPORTS_DEFINED) && \
!defined(CONDUIT_TU_IS_CUDA) && !defined(CONDUIT_TU_IS_HIP)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

  1. we should use special CONDUIT WINDOWS macro that exists somewhere
  2. This is a gaping wound for @JustinPrivitera to figure out. Can we move things back to the cpp?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is related to the discussion about RDC

@JustinPrivitera

Copy link
Copy Markdown
Member Author

TODO update changelog

@JustinPrivitera

Copy link
Copy Markdown
Member Author

TODO all transforms should provide a node/accessor to global execution options to get the right exec policy/output space.

@JustinPrivitera

This comment was marked as outdated.

@siramok

This comment was marked as outdated.

@JustinPrivitera

Copy link
Copy Markdown
Member Author

TODO address this concern: #1673 (comment)

siramok and others added 2 commits September 2, 2026 09:27
* Initial attempt at porting the new bm infra to device support branch

* Avoid a segfault by restricting generate_* to host only

* Initial commit

* Update plots

* Cleanup

* Revert sync change, make tests better

* More optimization

* Differentiate backends when plotting

* Make host_only a per-benchmark option

* Don't include deallocation in timing

* Rename take_data_ptr to assume_data_ptr

* Fix timing leak

* Don't copy into output buffers

* Initial commit

* Mirror changes in the pre-DE version of this PR

* Address feedback

* Make many improvements

* Caliper tutorial tweaks

* Initial commit

* Greatly simplify the API

* Add TODOs

* Add another TODO

* Make comments better

* Cleanup

* Add better comment

* Exploratory notes

* Remove resolved TODOs

* Remove a case that didn't make sense

* Finish porting

* Support DataArrays

* Polishing, better tests

* Update src/libs/conduit/conduit_execution_dispatch.hpp

Co-authored-by: Justin Privitera <35237779+JustinPrivitera@users.noreply.github.com>

* Address feedback

* Improve verbiage

* More polishing

* Use dispatches, some cleanup

* Always dispatch the non-polyhedral case

* Reorder things in anticipation of more overloads being added

* Redo unique_mask

* Polishing

* Add info to docs

* Remove TODOs

* Cleanup duplicated TODOs

* Remove LUTs

* Shorten header

* Update src/libs/blueprint/conduit_blueprint_mesh.cpp

Co-authored-by: Justin Privitera <35237779+JustinPrivitera@users.noreply.github.com>

* Remove maps

* Exercise other meshes and shape types

* Use correct policy querying

* Remove what I can

* Polish

* Make centroids test stricter

---------

Co-authored-by: Justin Privitera <35237779+JustinPrivitera@users.noreply.github.com>
@siramok

siramok commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

MSVC was throwing this compilation error for C++17:

D:\a\conduit\conduit\src\libs\blueprint\conduit_blueprint_mesh.cpp(1876,36): error C2131: expression did not evaluate to a constant

The code in question:

const index_t max_stack_npts = 32;
conduit::execution::forall(policy, 0, topo_num_elems, [=] CONDUIT_EXEC(index_t ei)
{
    ...
    index_t elem_coord_indices[max_stack_npts];

It seems that declaring max_stack_npts as const was not sufficient for MSVC, because the forall captures it by value [=]. Initializing stack arrays with a non-const expression is not allowed, but the Linux compilers are smart enough to know that our usage is valid, hence the error on Windows only.

The simple (smallest diff) way to fix this was to make max_stack_npts a compile-time constant using constexpr:

constexpr index_t max_stack_npts = 32;

It should also work to just move max_stack_npts into the forall body itself (which I will try next if this didn't fix CI).

@siramok

siramok commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

constexpr did not save us, so I have moved max_stack_npts into the forall body. Hopefully that does the trick.

@JustinPrivitera

Copy link
Copy Markdown
Member Author

constexpr did not save us, so I have moved max_stack_npts into the forall body. Hopefully that does the trick.

Thanks for fixing this. We also don't have to lambda capture by value (I think???). Maybe we could try that as well.

@siramok

siramok commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

We also don't have to lambda capture by value

I looked into it very briefly at one point, and my takeaway was that foralls would need to capture by value if they would ever be executed on device (host-only kernels didn't have to worry). I don't actually know if that's true.

But now that I have unified memory on the brain, I do wonder if 1) our kernels could capture by reference in unified memory and 2) if there's a kernel anywhere that would benefit from that.

* Initial attempt at porting the new bm infra to device support branch

* Avoid a segfault by restricting generate_* to host only

* Initial commit

* Update plots

* Cleanup

* Revert sync change, make tests better

* More optimization

* Differentiate backends when plotting

* Make host_only a per-benchmark option

* Don't include deallocation in timing

* Rename take_data_ptr to assume_data_ptr

* Fix timing leak

* Don't copy into output buffers

* Initial commit

* Mirror changes in the pre-DE version of this PR

* Address feedback

* Make many improvements

* Caliper tutorial tweaks

* Greatly simplify the API

* Add TODOs

* Add another TODO

* Make comments better

* Cleanup

* Add better comment

* Initial commit

* Better naming consistency

* Remove resolved TODOs

* Remove a case that didn't make sense

* Support DataArrays

* Polishing, better tests

* Port DataArray and DataAccessor to execution model

* Update src/libs/conduit/conduit_execution_dispatch.hpp

Co-authored-by: Justin Privitera <35237779+JustinPrivitera@users.noreply.github.com>

* Address feedback

* Improve verbiage

* More polishing

* Reorder things in anticipation of more overloads being added

* Polishing

* Address feedback

* Add info to docs

* Lazily cache active space

* Make set_value_helper private again

* Cache the execution policy instead of the memory space

* Support magic sets

* Add test comments

* Address feedback

* Add temp buffer comment

---------

Co-authored-by: Justin Privitera <35237779+JustinPrivitera@users.noreply.github.com>
@siramok

siramok commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Speaking of unified memory and whether we're handling it correctly, here's a link to my initial investigation on it: #1614 (comment)

@siramok

siramok commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

This job was abandoned. We have detected that logs from the agent may have not finished uploading. We have included our in-memory record of all log lines uploaded before we lost contact with the agent

CI failed after 90 minutes exactly, I wonder if we exceeding a time limit?

@siramok

siramok commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

For context, the PR before this most recent one (which does increase compile times) finished in 1h 0m 2s.

….com:LLNL/conduit into task/JustinPrivitera/05_01_24/device_support
@siramok

siramok commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Oh jeez, our in progress check was running for 14 hours...

I have restarted CI. If it fails again at 90 min then I think this issue may be above my pay grade.

@JustinPrivitera

Copy link
Copy Markdown
Member Author

This is really strange.

@siramok

siramok commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Do you have a button to manually stop CI? I suspect that this in progress check will continue running in perpetuity.

@siramok

siramok commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

[error] We stopped hearing from agent Azure Pipelines 5. Verify the agent machine is running and has a healthy network connection. Anything that terminates an agent process, starves it for CPU, or blocks its network access can cause this error. For more information, see: https://go.microsoft.com/fwlink/?linkid=846610

Since it happened almost exactly at 90 minutes again, this does feel like a timeout.

@siramok

siramok commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Cyrus says we should be able to run >90 minutes without an issue. The running out of space theory is looking plausible:

[warning] Free disk space on / is lower than 5%; Currently used: 98.11%

#1691 should alleviate this problem. Alternatively, I don't know if it's possible to increase the amount of space for the static build containers specifically (without adding more GH actions cost)?

Another thought: maybe rm -rf on the build folders after we install all of the tpls/deps would claw back enough space to enable conduit to finish installing? Although, if this is just executing ./build_conduit.sh then maybe that's not a viable solution.

@siramok

siramok commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

If we don't want to merge #1692 today, that's okay, but would someone with power mind stopping this endless CI run?

Merged!

…1692)

* Initial commit

* Whoops, forgot build_conduit.sh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

use cases + strawman interface to raja based host device exec interface

3 participants