search

StanfordLegion / legion

The Legion Parallel Programming System
https://legion.stanford.edu
Apache License 2.0
675 stars 146 forks source link

Review list of installed headers on make-based install #973

Open manopapad opened 3 years ago

manopapad commented 3 years ago

The following headers are not obviously project-internal (e.g. they're not named "foo_impl.h" or "foo_internal.h"), but are not included in the make-based install of Legion. We've come across at least one app (Lux) that was using one of them (realm/cuda/cuda_module.h), so I assume some of these are actually public and should be included in the install. Could the appropriate people take a look at this list and identify any missing public headers?

legion/field_tree.h
legion/garbage_collection.h
legion/interval_tree.h
legion/legion_allocation.h
legion/legion_analysis.h
legion/legion_c_util.h
legion/legion_context.h
legion/legion_instances.h
legion/legion_ops.h
legion/legion_ops.inl
legion/legion_profiling.h
legion/legion_profiling_serializer.h
legion/legion_spy.h
legion/legion_tasks.h
legion/legion_trace.h
legion/legion_utilities.h
legion/legion_views.h
legion/mapper_manager.h
legion/rectangle_set.h
legion/region_tree.h
legion/region_tree.inl
legion/runtime.h
realm/activemsg.h
realm/activemsg.inl
realm/bgwork.h
realm/bgwork.inl
realm/circ_queue.h
realm/circ_queue.inl
realm/cmdline.h
realm/cmdline.inl
realm/cuda/cuda_module.h
realm/cuda/cudart_hijack.h
realm/deppart/byfield.h
realm/deppart/deppart_config.h
realm/deppart/image.h
realm/deppart/inst_helper.h
realm/deppart/partitions.h
realm/deppart/partitions.inl
realm/deppart/preimage.h
realm/deppart/rectlist.h
realm/deppart/rectlist.inl
realm/deppart/setops.h
realm/dynamic_table.h
realm/dynamic_table.inl
realm/gasnet1/gasnet1_module.h
realm/gasnet1/gasnetmsg.h
realm/hdf5/hdf5_module.h
realm/id.h
realm/id.inl
realm/interval_tree.h
realm/interval_tree.inl
realm/kokkos_interop.h
realm/lists.h
realm/lists.inl
realm/llvmjit/llvmjit_module.h
realm/metadata.h
realm/module.h
realm/mpi/am_mpi.h
realm/mpi/mpi_module.h
realm/mutex.h
realm/mutex.inl
realm/network.h
realm/network.inl
realm/nodeset.h
realm/nodeset.inl
realm/numa/numa_module.h
realm/numa/numasysif.h
realm/openmp/openmp_module.h
realm/openmp/openmp_threadpool.h
realm/openmp/openmp_threadpool.inl
realm/operation.h
realm/operation.inl
realm/pri_queue.h
realm/pri_queue.inl
realm/procset/procset_module.h
realm/python/python_module.h
realm/sampling.h
realm/sampling.inl
realm/tasks.h
realm/threads.h
realm/threads.inl
realm/transfer/channel.h
realm/transfer/channel.inl
realm/transfer/channel_disk.h
realm/transfer/lowlevel_dma.h
realm/transfer/transfer.h
lightsighter commented 3 years ago

All those headers are obviously internal header files to me. Clients of Legion and Realm should never need any of those header files. The make build system includes only the header files that should be user-facing in the installation.

lightsighter commented 3 years ago

What exactly does Lux need out of cuda_module.h?

manopapad commented 3 years ago

These are the offending lines: https://github.com/LuxGraph/Lux/blob/master/pagerank/pagerank_gpu.cu#L269-L277. Lux seems to be using Realm::get_runtime() and Realm::RuntimeImpl::get_memory_impl from realm/runtime_impl.h (which is obviously internal), and Realm::Cuda::GPUFBMemory::alloc_bytes and Realm::Cuda::GPUFBMemory::get_direct_ptr from realm/cuda/cuda_module.h in order to allocate GPU memory inside a task.

lightsighter commented 3 years ago

@streichler Will need to say if he wants to allow that for Realm clients, but I believe the the answer is that they should use the instance allocation interface in instance.h and not be going through the side like that.

Since this is a Legion program, they should be using the DeferredBuffer interface to allocate memory that is local to the task, which will go through the accepted Realm instance.h interface.

manopapad commented 3 years ago

I believe this memory is not task-local; it needs to persist for the lifetime of the app. Possibly the instance.h interface, or the new Legion variable-size allocation API are proper ways to do this. I made a corresponding issue on the Lux bug tracker: https://github.com/LuxGraph/Lux/issues/10.

streichler commented 3 years ago

Reopening because I need to do a scrub of the Realm headers at least.

lightsighter commented 3 years ago

For completeness, I've officially vetted the list of Legion header files and guarantee that the ones the Make build currently installs are precisely the user facing ones, no more and no less. I've also been curating the Realm list to do the same thing, but it's possible that I've misinterpreted some header files.

Side question: do we want to create a related issue to teach the CMake build system to do the same thing? I find it annoying that CMake moves my internal header files into the 'include' directory so users can link against internal interfaces.