Disable HIP_PLATFORM auto-detect if already defined. #130
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stellaraccident
requested review from
chrispaquot,
gandryey,
saleelk,
mangupta and
rakesroy
as code owners
stellaraccident
force-pushed
the
disable_hip_platform_auto_detect
branch
from
d6d4f27 to
832a770
Compare
This code appears to be old copy-pasta and is effectively just a no-op if -DHIP_PLATFORM=amd is passed explicitly. It has a number of nit-picky checks which will fail in this case for no real reason. This is actually worse than that because it leaks global variables that establish a different default value/scoping vs the definitions in the inner hipamd/CMakeLists.txt. I opted to rewrite this section to scope the detection logic and keep it from leaking with respect to the inner build file. The detection logic is still not great but at least will not actively cause damage or make control flow hard to reason about. It seems like this is for legacy/compatibility so I opted to simply sequester it vs applying more diligence to it. Also fixes a bug in hip-config.cmake where it enforces that hipcc exists even if not taking the install branch in the code above. See: nod-ai/TheRock#21
stellaraccident
force-pushed
the
disable_hip_platform_auto_detect
branch
from
832a770 to
54553db
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This code appears to be old copy-pasta and is effectively just a no-op if -DHIP_PLATFORM=amd is passed explicitly. It has a number of nit-picky checks which will fail in this case for no real reason.
This is actually worse than that because it leaks global variables that establish a different default value/scoping vs the definitions in the inner hipamd/CMakeLists.txt. I opted to rewrite this section to scope the detection logic and keep it from leaking with respect to the inner build file. The detection logic is still not great but at least will not actively cause damage or make control flow hard to reason about. It seems like this is for legacy/compatibility so I opted to simply sequester it vs applying more diligence to it.
Also fixes a bug in hip-config.cmake where it enforces that hipcc exists even if not taking the install branch in the code above.
See: nod-ai/TheRock#21