-
Notifications
You must be signed in to change notification settings - Fork 73
[HIP-VS][refactor] VS support for multiple HIP SDK versions #203
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
base: amd-staging
Are you sure you want to change the base?
Conversation
schung-amd
commented
Feb 26, 2025
- Add HIP SDK version detection to .vcxproj files via HIP_PATH environment variable and associated .hipVersion file
- Import the appropriate toolset properties and targets as default
- Import additional toolset properties and targets to prevent property page in VS from breaking when another toolset is selected
- Add build exclusion for toolsets which do not match the detected HIP SDK version
- Adds associated documentation to the README providing guidance on how to use the VS files when multiple HIP SDK versions are installed
This reverts commit 70dfe45.
Removed hardcoded additional toolset imports as original code works here (detecting HIPVersion by stripping from the selected platform toolset name). |
Considering replacing the hardcoded build exclusion for non-matching HIP SDK toolset and compiler with a warning, it seems like there are some internal uses for this. Will also need to patch the new hipFFT examples. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Major concerns]
I understand your necessity to support so many vcxproj and sln files in ROCM-examples, but:
- The changes like this one might be only considered in HIP-VS, as the HIP-VS Extension generates Visual Studio projects. Manual changes in the derived projects are unacceptable and harmful.
- The fact that HIP-VS and the derived and generated HIP-VS projects should have the same versioning has been discussed in AMD, and the decision was that we do not want to support N-N forward and backward compatibility across different versions of ROCm/HIP components, including the derived ones like ROCm-Examples.
[Reasons]
- 1-1 compatibility of all the ROCm components within a single ROCm version is much more straightforward to support, and it is what we guarantee to the users and customers by saving the integrity within a single version.
- The same version paradigm is used by the NVIDIA CUDA Development Toolkit.
- We can't guarantee N-N forward and, moreover, backward compatibility across different ROCm components, core or derived ones.
[Existing solution][ROCm-Examples only]
If needed (urgently by customers), there always exists an opportunity to switch the Platform Toolset for the HIP/ROCm VS project(s) to a higher or lower version, as long as to switch the HIP SDK version. But for their own risk. We don't guarantee either integrity or correctness.
Btw, the same paradigm (switching Platform Toolset) is used by Microsoft itself.
[Minor Concerns]
Please find in the review itself.
I'm open to discussing HIP-VS changes, but I can't afford to ship this change in life.
<PropertyGroup Condition="'$(PlatformToolset)'!='HIP nvcc $(HIPPathVersion)' and '$(PlatformToolset)'!='HIP clang $(HIPPathVersion)'"> | ||
<ProjectExcludedFromBuild>true</ProjectExcludedFromBuild> | ||
</PropertyGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below: unrelated change which has to be i a separate PR.
<HIPVersionFileContents>$([System.IO.File]::ReadAllText('$(HIP_PATH)bin\.hipversion'))</HIPVersionFileContents> | ||
<HIPVersionMajor>$(HIPVersionFileContents.Substring($([MSBuild]::Add($(HIPVersionFileContents.LastIndexOf("HIP_VERSION_MAJOR")),18)),2).Trim())</HIPVersionMajor> | ||
<HIPVersionMinor>$(HIPVersionFileContents.Substring($([MSBuild]::Add($(HIPVersionFileContents.LastIndexOf("HIP_VERSION_MINOR")),18)),2).Trim())</HIPVersionMinor> | ||
<HIPPathVersion>$(HIPVersionMajor).$(HIPVersionMinor)</HIPPathVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solution is to discuss with HIP-VS team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, let's discuss this offline at a later point in time.
Thanks for the detailed response. This may not be the right approach/implementation right now, but the state of the VS files in the ROCm examples is unacceptable. I understand the HIP SDK versioning issue has been discussed previously, but as the new maintainers of the examples we haven't had a chance to participate in this discussion, and I would like to revisit it. While ROCm example compatibility is not guaranteed for every version, this is nothing new; users are allowed to download the examples and try to run them on any ROCm version on Linux, so why are Windows users unable to even open the project files if their versions don't match? Not to mention there is no clear indication of which release branch to use for a given HIP SDK version in the first place; the recent issue with the 6.2.4 release branch not supporting HIP SDK 6.2.4 is an example of this. If version compatibility is strictly enforced, it makes more sense to excise the VS files from this repo altogether and provide them in a separate package or HIP SDK-specific release branch, as the versioning will never line up properly with the mainline versioning to begin with. Open to discussing with the HIP VS team. We (the HIP VS team and our team) should also have a discussion about the HIP VS project file generation process so that we're on the same page, I don't think we have information about this process. |
Could you be more concrete here in describing their state?
They are able, and I explained how it works. Projects created for HIP SDK 6.2.x can be opened for the newer version(s) of HIP SDK, 6.3.x, for example. HIP SDK can be changed in PPP as well. Did you encounter some issues with opening HIP projects in Visual Studios? Could you provide examples?
The release schedule of HIP SDK aligned with Linux ROCm is another organizational problem discussed multiple times. I believe it should be fixed at its layer - organizational level, not in the Software itself. Let's not mix problems.
@hhalilovAMD, @kzhuravl, @searlmc1, please let me know how we can arrange that. Thanks! |
I think the way mismatched HIP SDK and VS project file version is handled (that is to say, not at all) is ugly. Unless I'm missing some documentation it is not stated what HIP SDK version a given release requires, so a user might find they have wasted their time downloading the wrong rocm-examples files or installing the wrong HIP SDK version. The first indication that you have the wrong HIP SDK version should not be Visual Studio complaining that it can't find the toolset for some other HIP SDK version. We should ensure we're communicating clearly which HIP SDK version is compatible with a given VS project in advance so users cannot be surprised by this. This can be pointed out in docs but I'd prefer if it was indicated in the files themselves somehow (e.g. in the filename). I also don't think the VS project files line up completely with how we're triaging and resolving GitHub user issues. If the HIP VS project files are untouchable until regenerated by the HIP-VS side, this seems like an additional barrier to resolving issues that may arise. This is just an inconvenience, but I would prefer if we were permitted to patch these files by hand to resolve issues in staging. Of course, larger changes like this PR would go through the HIP-VS team first before merging. Open to discussion on this. A less invasive solution would be to figure out a better way to incorporate the VS project files into the development and release environment, either through changing the release channel (e.g. dedicated release branches for HIP SDK versions) or thorough documentation.
If we're letting users do this already, then is the solution in this PR really not sane? All it does is automatically set the platform toolset to the HIP SDK version the user currently has set in their environment variables. This would be more convenient on the user end, as they wouldn't have to have multiple HIP SDK versions installed to do this. Is there something I'm missing?
Switching the platform inside Visual Studio does work when you have multiple versions of the HIP SDK installed, but this requires users to know what HIP SDK version they need to have installed in the first place. As outlined above I don't think this is fair to expect at the moment. That being said, I have not done testing with HIP SDK 6.3, so if this is solved there I apologize, but from what you've said it sounds like users still need the old HIP SDK version installed. Also, could just be a bug on my end, but in my experience switching the toolset does not change the compiler that is used. That seems to depend only on the value of the HIP_PATH environment variable, which is cached by MSBuild upon process launch. This value is set by the installer but always overwrites, so the most recently installed HIP SDK version takes precedence. If a user installs HIP SDK 6.3, then realizes they currently need HIP SDK 6.2 to run the examples and downloads that version, Visual Studio will use the compiler from 6.2 until the user changes this value themselves or installs a new HIP SDK version. In this scenario, when we update the VS project files to HIP SDK 6.3, they will not reinstall (as they already have HIP SDK 6.3 installed) and might be unknowingly using the old compiler. In this way, users with multiple HIP SDK versions installed are not even guaranteed to be using a configuration we've tested. I had a guard against this in the PR to check for compiler version mismatch, but probably needs to be a warning instead, as it seems there is some internal testing done with mismatching compiler and HIP-VS toolset version. We could also just add documentation as developers should be capable of setting the environment variable if they know this could be an issue in advance, but it's awkward to have to relaunch the process to capture the new value in the first place. It would be better if this was solvable on the VS extension side but I don't know if that's possible. Up to you how you want to handle this. Finally, I've also run into issues on one test system where multiple HIP-VS extensions could not be installed at the same time. It sounds like this is a bug, but on that system I had to reinstall HIP SDK every time I wanted to change versions, as the installer did not have an option to only install the HIP VS extension. I could probably build the extension from source to avoid this, but as the HIP-VS repo is private at the moment there is no alternative for users that I'm aware of.
Sure, that makes sense, but in the meantime let's see if we can reduce any potential friction on the user end that might arise from our awkward release cadence. |