-
-
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
Changes from all commits
f579c74
f136752
d6adc4b
41f9461
e240329
e15c449
f4f68f4
d276a67
73e919d
7ff6984
e30bae6
45b3242
7bbde17
32b9f37
c3848d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,7 +65,9 @@ stdenv.mkDerivation (finalAttrs: { | |
|
||
# Really strange behavior, `#!/usr/bin/env perl` should work... | ||
substituteInPlace CMakeLists.txt \ | ||
--replace "\''$ \''${hipify-perl_executable}" "${perl}/bin/perl ${hipify}/bin/hipify-perl" | ||
--replace "\''$ \''${hipify-perl_executable}" "${perl}/bin/perl ${hipify}/bin/hipify-perl" \ | ||
--replace-warn "-parallel-jobs=12" "-parallel-jobs=1" \ | ||
--replace-warn "-parallel-jobs=16" "-parallel-jobs=1" | ||
Comment on lines
+69
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the reason for those changes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To respect |
||
''; | ||
|
||
postInstall = lib.optionalString buildTests '' | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please elaborate a bit to explain the changes in this file. Why is it not necessary anymore to call the tensile build separately for each GPU generation and merge the results? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the reason for this was that without it
This is something we could address via #305920 or something like e380b53, but I don't know if we need a solution for the size issue before merging this, since we're not making things worse than they are right now anyways. I have it on my list to check the sizes of other outputs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say merge this for now then fix it later, since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it broken on master because of cmake? Seems like we don't want to introduce a cache miss just because it's broken on master. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, it's fixed on master now. If we merge this PR as is I think a better argument might be that not having
Probably that argument is also invalid to some degree, because Is that right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am now checking if removing the early-engineering sample only gfx940 and gfx941 targets from just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this would get us down to 2.7GB: 89ab15f |
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 commit says it is is written by Cordell Bloor and signed-off by you.
Where is this from?
Are we allowed to include this?
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 wrote the patch for Debian.
https://salsa.debian.org/rocm-team/rocm-hipamd/-/blob/debian/5.7.1-3/debian/patches/0025-improve-rocclr-isa-compatibility-check.patch
Yes.
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.
Thank you @cgmb! @mschwaig to prevent confusion, I've now switched the patches' url to Debian whenever possible. The only ones that are not switched are those that don't apply cleanly and needs manual fixes/rebase.
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 looks good.
For the ´rocclr isa compatibility check´, are you aware that the new patch is slightly different?
Just asking to make sure this is intended.
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.
Yes, I'm aware. The new patch is just being more cautious on compatibility by checking both the code object ISA and the host ISA. The difference is intended.