-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
rocmPackages: extend ISA compatibility #298388
Conversation
1e0e3e3
to
82af478
Compare
I think having that artificial barrier removed is a great change. I just want to give you a heads up after quickly looking over that linked email: It seems those patches are originally provided as git commits. Unless they need to be adapted somehow form the original commits, it would be better to integrate them by using |
@mschwaig Updated. Thank you for your comment! |
I will leave |
Well I guess that had to be 2 nights because it ran out of space building |
366adb5
to
fa1628b
Compare
Going by the output of Result of 14 packages marked as broken and skipped:
6 packages failed to build:
34 packages built:
Here is the build log:
Did you manage to run |
@mschwaig Thank you for the run! I haven't run I had a fix for rocBLAS but it was really messy. I think I figured out a clean solution that requires just one short patch and no more |
Quick question: I want to test rocBLAS through llama.cpp. How do I build a |
Should be That works as part of some shell or system config. I don't know by without looking it up myself how to do it directly on the command line.
|
Hi @mschwaig would you mind running |
4798380
to
2c585f5
Compare
Great to hear that it compiles. I'm running |
I was reading the llvm documentation for the upcoming generic targets and it doesn't look like that this patch will always work, for example: |
Result of 14 packages marked as broken and skipped:
5 packages failed to build:
35 packages built:
These are the logs of the failing packages: The logs for |
2c585f5
to
13e1f01
Compare
Hi @Tungsten842, I believe it's actually the reverse. The document is saying that because we're compiling against Therefore, |
@mschwaig I believe I have fixed the issue (the header file |
Makes sense. But there is still one problem, it looks like that gfx900 supports some v_mad_mix instructions. But (gfx906, gfx904...) do not. |
@Tungsten842 You're right, though this PR doesn't make those changes, only the following:
So I believe we should be in the safe zone. |
Signed-off-by: Gavin Zhao <[email protected]>
…cblas Signed-off-by: Gavin Zhao <[email protected]>
Signed-off-by: Gavin Zhao <[email protected]>
Signed-off-by: Gavin Zhao <[email protected]>
Signed-off-by: Gavin Zhao <[email protected]>
Signed-off-by: Gavin Zhao <[email protected]>
Signed-off-by: Gavin Zhao <[email protected]>
Signed-off-by: Gavin Zhao <[email protected]>
Signed-off-by: Gavin Zhao <[email protected]>
Great! I already ran Result of 10 packages marked as broken and skipped:
42 packages built:
The sizes are looking good as well:
EDIT: llama-cpp also still runs on my RX 6600 |
…PU targets Signed-off-by: Gavin Zhao <[email protected]>
062f308
to
32b9f37
Compare
@cgmb Good call, thanks for pointing that out! I added @mschwaig I've removed |
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.
I used git range-diff master 062f308b9601f3fec588c8df0225eff7731f9834 32b9f37d57826364db0bc579ee75fa82af1fd623
to verify the contents of that last force-push and it looks good.
I would prefer if we had more info about what targets are missing in that list and why.
Maybe we should have a comment above that gpuTarget
simply stating that
these are the defaults with gfx940
and gfx941
removed because they are early engineering samples and gfx1012
removed, because of the patch that makes it unused.
Otherwise it will be difficult to get this list right on future ROCm updates.
…uilt Signed-off-by: Gavin Zhao <[email protected]>
I will take a closer look at this tomorrow and see if we can land this. Thanks a lot for working on this! |
The changes looks good. I'm just running a last Again thank you @GZGavinZhao and @mschwaig for making this happen! |
Description of changes
This will eliminate the need for users to use
HSA_OVERRIDE_GFX_VERSION
to emulate their GPUs as a supported GPU model.For more info, see this mailing thread.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.