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

Use PACKAGE_PREFIX_DIR rather than hard coded ROCM_PATH for finding the config files #18

Closed
wants to merge 1 commit into from

Conversation

raramakr
Copy link
Contributor

No description provided.

@jrmadsen
Copy link
Contributor

@raramakr please place ${ROCPROFSDK_PACKAGE_PREFIX_DIR} before the hardcoded paths instead of replacing them. Your changes incorrectly assume that all the packages will always be installed under the same prefix, some package managers like spack have unique paths for each package.

jrmadsen

This comment was marked as outdated.

Copy link
Contributor

@jrmadsen jrmadsen left a comment

Choose a reason for hiding this comment

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

@raramakr
Copy link
Contributor Author

@raramakr please place ${ROCPROFSDK_PACKAGE_PREFIX_DIR} before the hardcoded paths instead of replacing them. Your changes incorrectly assume that all the packages will always be installed under the same prefix, some package managers like spack have unique paths for each package.

HI @jrmadsen , The patch was raised to remove the hard coded paths. To make ROCm relocatable , need to get rid of hard coded paths.
Even If each package have unique paths(not sure it applies to ROCm in spack) , having a hard coded path like /opt/rocm-ver will not help to find the package

@jrmadsen
Copy link
Contributor

jrmadsen commented Sep 24, 2024

HI @jrmadsen , The patch was raised to remove the hard coded paths. To make ROCm relocatable , need to get rid of hard coded paths. Even If each package have unique paths(not sure it applies to ROCm in spack) , having a hard coded path like /opt/rocm-ver will not help to find the package

Even as it is, this is relocatable, these “hard coded paths” are configured from where the build found these packages. They exist to help ensure that the install tree find_package, with no additional hints (e.g. CMAKE_PREFIX_PATH, <PackageName>_DIR), falls back to searching for the package where the build of rocprofiler-SDK found the packages. If these paths don’t exist in these locations, they are simply ignored. If you review the config search procedure you can see the HINTS and PATHS are the 4th option — these paths are nothing more than fallbacks.

@raramakr raramakr requested a review from frepaul September 24, 2024 17:24
@raramakr
Copy link
Contributor Author

raramakr commented Oct 2, 2024

Closing the PR . After discussion will raise a new PR in internal branch

@raramakr raramakr closed this Oct 2, 2024
ammarwa added a commit that referenced this pull request Dec 6, 2024
* Adding changes to register and read symbols from the hip fat binary

* adding json output for host_functions

* added error handling

* adding json tool support

* Adding tests

* formatting changes

* Adding documentation

* refactoring as per amd-staging

* Adding intializers and changing macros

* Fix page-migration background thread on fork (#31)

* Fix page-migration background thread on fork

After falling off main in the forked child, all the children
try to join on on the parent's monitoring thread. This results
in a deadlock. Parent is waiting for the child to exit, but
the child is trying to join the parent's thread which is
signaled from the parent's static destructors.

Even with just one parent and child, due to copy-on-write
semantics, a child signalling the background thread to join
will still block (thread's updated state is not visible
in the child).

This fix creates background treads on fork per-child with a
pthread_atfork handler, ensuring that each child has its own
monitoring thread.

* Formatting fixes

* Detach page-migration background thread and update test timeout

* Attach files with ctest

* Update corr-id assert

* Tweak on-fork, simplify background thread

* Revert thread detach

* Adding --collection-period feature in rocprofv3 to match v1/v2 parity (#9)

* Adding Trace Period feature to rocprofv3

* Adding feature documentation

* Update source/bin/rocprofv3.py

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Fixing format

* Moving to Collection Period and changing the input params

* Format Fixes

* Fixing rebasing issues

* Removing atomic include from the tool

* Adding more options for units, optimizing the code

* Fixing rocprofv3.py

* Fixing time conv & adding time controlled app

* Fixing format

* Changing to shared memory testing methodology

* use of shmem use

* Fix include headers for transpose-time-controlled.cpp

* Format upload-image-to-github.py

* Removing shmem and using only env var to dump timestamps from the tool

* Tool Fixes + Test Config

* Adding Tests

* Fixing Review comments

* Update trace period implementation

* Update trace period tests

* check between start and stop timestamps

* Merge Fix

* Update validate.py

* Improve safety of rocprofiler_stop_context after finalization

* Pass context id to collection_period_cntrl by value

* Adding 20 us error margin

* Ensure log level for collection-period test is not more than warning

---------

Co-authored-by: Ammar ELWazir <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jonathan R. Madsen <[email protected]>

* Update lib/rocprofiler-sdk/code_object/hip/code_object.*

- move error code check macros to implementation
- fix macros which check error code
- use constexpr values instead of #define

* Update lib/rocprofiler-sdk/code_object/hip/code_object.*

- debugging for error that cannot be locally reproduced

* Update lib/rocprofiler-sdk/code_object/hip/code_object.*

- improve error handling and logging

* Update lib/rocprofiler-sdk/code_object/hip/code_object.*

- tweak to non-fatal logging messages

* Update lib/rocprofiler-sdk/code_object/hip/code_object.*

- cleanup of logging messages

* Update host kernel symbol register data fields

* Update source/lib/rocprofiler-sdk/code_object/hip/code_object.hpp

---------

Co-authored-by: Madsen, Jonathan <[email protected]>
Co-authored-by: Kuricheti, Mythreya <[email protected]>
Co-authored-by: Elwazir, Ammar <[email protected]>
Co-authored-by: Ammar ELWazir <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jonathan R. Madsen <[email protected]>
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.

2 participants