Better handle building rocSPARSE with ROC-TX

[jsandham]

Summary of proposed changes:

• Better handle the situation when we are trying to build rocsparse with roctx instrumentation, but the roctx shared library does not exist in /opt/rocm/lib

[YvanMokwinski]

find_path and find_library should be required in the case of BUILD_ROCTX, and should be placed in the root cmake script.

[cgmb]

roctracer is eventually going to be replaced by the rocprofiler-sdk. There is also roctx provided by the rocprofiler-sdk. At some point, you'll need to support that roctx as well.

You can find that version of roctx via find_package(rocprofiler-sdk-roctx PATHS ${ROCM_PATH} /opt/rocm) and linking to the rocprofiler-sdk-roctx::rocprofiler-sdk-roctx target.

It's been used in hipfort for a while.

[cgmb]

I have not tested this (as I expect that you've done that), but it looks reasonable to me.

[cgmb]

This is generally something that is nice to be able to control explicitly. My favourite behaviour is -DBUILD_WITH_ROCTX=ON means error if roctx not found, -DBUILD_WITH_ROCTX=OFF means don't use roctx, and unspecified means search for roctx and use it if found (but don't error if it's not found).

With that said, the way you've done it will work fine for me. I don't actually need finer-grained control.

[stellaraccident]

Folks, we need to revert this patch. I get that you are trying to make this specific project better but we aren't adding more project specific find modules to work around bugs in our own software as it just makes the overall situation worse. (In this case: the bug is that roctracer does not export a proper package and should)

If you must adapt to the current state of affairs and be more robust, copy and paste the logic that MIOpen uses. Otherwise, this needs to be fixed upstream at the source.

[cgmb]

@stellaraccident, could you clarify what concrete problems this is creating?

I get that you are trying to make this specific project better

Precisely the opposite. The point was to implement something that could be standardized, rather than having every library write their own dozen lines of CMake code for this.

If you must adapt to the current state of affairs and be more robust, copy and paste the logic that MIOpen uses.

I don't understand why the MIOpen logic is better. The find module does the same thing, but encapsulates the logic into a standardized mechanism that can be copied as a single file, rather than a bunch of lines to copy/paste/modify.

this needs to be fixed upstream at the source

I would agree, however roctracer's roctx is considered deprecated and rocprofiler-sdk's roctx has a CMake config file, so it's considered fixed upstream. The problem is that we can never fix the past. Even if we fix roctracer today, there will be many working versions in the wild that will never have that feature. At some point, it would make sense to set rocprofiler-sdk as the minimum requirement for building the mathlibs, but just not yet...

[stellaraccident]

@stellaraccident, could you clarify what concrete problems this is creating?

We're in the process of migrating to both the monorepo and single invocation builds of projects in this stack. Having component specific find modules for things in our own build graph creates multiple sources of truth for what a package "is", as the first one wins and claims the namespace for all others. If we start landing these now, I'll just have to revert them with an LSC in a couple of weeks and implement something that has one definition for the project to be sound.

I get that you are trying to make this specific project better

Precisely the opposite. The point was to implement something that could be standardized, rather than having every library write their own dozen lines of CMake code for this.

If you must adapt to the current state of affairs and be more robust, copy and paste the logic that MIOpen uses.

I don't understand why the MIOpen logic is better. The find module does the same thing, but encapsulates the logic into a standardized mechanism that can be copied as a single file, rather than a bunch of lines to copy/paste/modify.

It is local and does not pollute the shared public namespace.

this needs to be fixed upstream at the source

I would agree, however roctracer's roctx is considered deprecated and rocprofiler-sdk's roctx has a CMake config file, so it's considered fixed upstream. The problem is that we can never fix the past. Even if we fix roctracer today, there will be many working versions in the wild that will never have that feature. At some point, it would make sense to set rocprofiler-sdk as the minimum requirement for building the mathlibs, but just not yet...

The migration plan for rocprofiler-sdk has no definite timeline and could easily extend to this time next year because it has to account for multiple framework level migrations before we can reasonably redirect the projects to the new marker API (the compatibility goes one way: markers made with the old roctracer API will show up in rocprofv3 based profiler tooling, but not the other way around. The most used profilers are in the frameworks and will not even start migration to rocprofv3 for a further indefinite time period, to say nothing of the amount of time it will take for them to release enough new stable versions that let us make the change permanent).

We fix software: we should add the export to roctracer and give it a graceful retirement. Fixing it there and rolling that forward will give us a much more robust migration path for this stuff, since at the appropriate time, we can just start having rocprofiler-sdk advertise roctx itself, removing roctracer from ROCm and achieve an atomic source level switch. Just because something is deprecated or being no longer invested in does not mean we just ignore it. Send me a patch to add the exports to roctracer that should have been there all along, and I'll approve it and release it to mainline. If no one else does this, I'll put it on my team's list for the coming weeks. Then once we are in the monorepo, we will do one LSC to fix this in every math-lib simultaneously.

[cgmb]

@stellaraccident, I still don't entirely follow on how this find module impacts other libraries in an add_subdirectory context. Ultimately, this is not a big deal either way, so if you feel strongly about it, then I'm not going to argue. I just don't understand the underlying problem.

It is local and does not pollute the shared public namespace.

The MIOpen snippet creates the ROCTRACER_INCLUDE_DIR and rocTracer cache variables, which are both global. (And the latter is misleading, as the variable contains the path to the roctx library, not the roctracer library, but I digress...)

I suppose what I'm saying is, aren't they both global in mostly the same ways?

we should add the export to roctracer and give it a graceful retirement.

I mean, I tried opening a small PR last year, mostly to check if it was worth trying to upstream patches. I was informed that they weren't making changes to roctracer anymore unless it was really important. If you're able to get changes into roctracer, then I support that.

[stellaraccident]

I mean, I tried opening a small PR last year, mostly to check if it was worth trying to upstream patches. I was informed that they weren't making changes to roctracer anymore unless it was really important. If you're able to get changes into roctracer, then I support that.

Let's focus here as all of the downstream hacks are not great and trying to slice which one is less great probably doesn't get us very far.

I can get patches into roctracer. We have to maintain our software until it is actually no longer depended on, not just when we lose interest in it. We're going to be using roctracer for a long time and should make this pragmatic fix that rationalizes the whole stack.