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

Develop Stream - Fix ROCm 6.2.2 build error #183

Merged
merged 5 commits into from
Dec 16, 2024

Conversation

Beanavil
Copy link
Collaborator

@Beanavil Beanavil commented Nov 29, 2024

This PR updates the project to use ROCm 6.2.2 and tackles the internal ticket regarding llvm_ir_to_executable and assembly_to_executable examples not building on Windows.

The root cause of the issue was that a commit in llvm-project had made some modifications to the linking. I saw that there was a PR to fix this in Linux previously (#142) but it was later reverted (#161) because a fix for that linking-related commit had been added in llvm-project. However, said fix was not released as part of ROCm 6.2.2 (nor 6.2.4 as far as I'm aware), so this build error is still present in both Linux and Windows. Therefore, this PR brings back the fix for Linux from the PR mentioned previously and also adds the fix for Windows. These have been tested in our internal CI and no build errors have been observed.

FYI: this branch is based on develop_stream_20241129, so I set up the PR to target that branch instead of develop for a better review experience. Reviewers should keep in mind that PR #182 should be merged before this one, and then the target branch should be changed to develop.

@Beanavil
Copy link
Collaborator Author

I see that the pipeline is not run because the current target branch is not any of the ones set up to trigger the actions, so perhaps after review (and before merging #182) I can change the target branch to develop so that it can be double-checked that the build is indeed fixed.

Copy link
Collaborator

@MKKnorr MKKnorr left a comment

Choose a reason for hiding this comment

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

The 6.2.2/6.2.4 releases already include the patches for the assembly/llvm examples, merging it to develop now would just reintroduce this bug, as it was just a workaround for a compiler change that has been fixed in ROCm 6.3.

Please update the docker/CI ROCm versions to 6.3.

In order to run the assembly/llvm examples with the 6.2.2 or 6.2.4 releases you'll need a corresponding ROCm version. Granted, usually minor or patch versions should not include breaking changes, but in this case this was unavoidable due to changes in the compiler

@Beanavil Beanavil force-pushed the develop_stream_fix_6.2.2_build branch from cb1f005 to 66f19ba Compare December 13, 2024 17:01
@Beanavil Beanavil requested a review from a team as a code owner December 13, 2024 17:01
@Beanavil Beanavil changed the base branch from develop_stream_20241129 to develop December 16, 2024 08:16
@Beanavil
Copy link
Collaborator Author

@MKKnorr ah I see now, that ROCm 6.3.0 has the fix for the compiler issue. I only left the fixes in the llvm/assembly examples for Windows because the latest SDK is still not ROCm 6.3 or greater.

@Beanavil
Copy link
Collaborator Author

Hmm I see some weird test failures in the azure pipeline for the assembly/llvm examples:

An error encountered: "no kernel image is available for execution on the device"

As far as I see, this is not coming from the changes in this PR, because these were happening in several other pipelines for previous PRs. Do you know where does this come from? We are also not getting these in our internal pipeline.

@MKKnorr
Copy link
Collaborator

MKKnorr commented Dec 16, 2024

Weird, I also can't reproduce this error locally. But I agree, there does not seem to be anything in this PR that would cause this, I'll open a ticket to investigate this

@MKKnorr MKKnorr merged commit 31e7ba9 into ROCm:develop Dec 16, 2024
1 of 3 checks passed
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