Bug 2213372 - Review Request: hipcc - HIP compiler driver
Summary: Review Request: hipcc - HIP compiler driver
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Tom Rix
QA Contact: Fedora Extras Quality Assurance
URL: https://github.com/ROCm-Developer-Too...
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2023-06-08 00:52 UTC by Jeremy Newton
Modified: 2024-01-10 19:38 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2024-01-10 19:38:10 UTC
Type: ---
Embargoed:
ppisar: fedora-review?


Attachments (Terms of Use)

Description Jeremy Newton 2023-06-08 00:52:51 UTC
Spec URL: https://mystro256.fedorapeople.org/hipcc.spec
SRPM URL: https://mystro256.fedorapeople.org/hipcc-5.5.1-1.fc39.src.rpm
Description:
hipcc is a compiler driver utility that will call clang or nvcc, depending on
target, and pass the appropriate include and library options for the target
compiler and HIP infrastructure.
Fedora Account System Username: mystro256
Copr Build: https://copr.fedorainfracloud.org/coprs/mystro256/rocm-hip/build/6019100/

Background:
Right now hipcc and hipcc.pl are located in the hip subpackage of rocclr. This hipcc package contains hipcc.bin, which is a c++ port of the perl script hipcc.pl.
Upstream is planning to move hipcc.pl to this source and deprecate hipcc.pl, so I'm just packaging this to get ready. In the future, when the hipcc.pl is moved, I will make changes to the rocclr package accordingly.

Comment 1 Tom Rix 2023-06-08 12:33:09 UTC
I think it is good to prepare for the deprecation of hipcc.pl.
I understand the need for hipcc.bin and hipconfig.bin but the extension is confusing
It would be best if hipcc and hipp.pl could go away and hipcc.bin became hipcc

calling hipcc.bin and hipconfig.bin directly both failed.

could this wait until the deprecation of hipcc.pl is completed upstream ?


Some other notes below

-Source0:        https://github.com/ROCm-Developer-Tools/HIPCC/archive/refs/tags/rocm-%{version}.tar.gz#/%{name}-%{version}.tar.gz
+# use %{url} to reduce generally all these instances that have it as a prefix
+#Source0:        https://github.com/ROCm-Developer-Tools/HIPCC/archive/refs/tags/rocm-%{version}.tar.gz#/%{name}-%{version}.tar.gz
+Source0:        %{url}/HIPCC/archive/refs/tags/rocm-%{version}.tar.gz#/%{name}-%{version}.tar.gz
 
 #Upstream fixes:
 # HIPCC is very new, so cmake was incomplete when 5.5 was tagged:
@@ -31,6 +33,8 @@ Requires:       hip >= %{version}
 Requires:       clang
 # 16.2 has an important fix for hipcc to work out of the box:
 Requires:       rocm-device-libs >= 16.2
+# battling hipcc vs hipcc.bin
+# Conflicts:      hip <= 5.5.1
 
 # This package depends on "hip" which is only built on these arches right now:
 ExclusiveArch:  x86_64 aarch64 ppc64le
@@ -51,12 +55,25 @@ must ensure the compiler options are appropriate for the target compiler.
 sed -i '/" -isystem " + hsaPath + "\/include"/d' src/hipBin_amd.h
 
 %build
-%cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo
+# Should just use %cmake, and let rpm macros determine the build type
+# %cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo
+%cmake 
 %cmake_build
 
 %install
 %cmake_install
 
+# Needs a %check
+# > hipcc.bin
+# sh: line 1: /tmp/canRunUUdQtd: Is a directory
+# sh: line 1: /tmp/canRuntxtm5v: Is a directory
+# sh: line 1: /tmp/canRunJMKATa: Is a directory
+# sh: line 1: /tmp/canRunhWTbpf: Is a directory
+# Device not supported - Defaulting to AMD
+# No Arguments passed, exiting ...
+#
+# Similar for hipconfig.bin
+

Comment 2 Jeremy Newton 2023-06-08 13:26:27 UTC
> It would be best if hipcc and hipp.pl could go away and hipcc.bin became hipcc

Yeah I believe that is the plan.

> could this wait until the deprecation of hipcc.pl is completed upstream ?

I think, but don't quote me on this, the plan is:
5.6 > add hipcc package with hipcc.bin as alternative to perl
5.7 > move hipcc.pl/hipcc to hipcc package, deprecate perl and default to hipcc.bin
6.0 > drop perl, delete hipcc, rename hipcc.bin to hipcc
Same idea with hipconfig{,.pl,.bin}
Anyway, I can hold on to this until 5.6 or 5.7 if you want, I just wanted to prepare in case there's issues.

> +# use %{url} to reduce generally all these instances that have it as a prefix

Good idea, I should do this to other ROCm packages too.

> +# battling hipcc vs hipcc.bin
> +# Conflicts:      hip <= 5.5.1

This isn't quite right. "hipcc" is just a wrapper script that calls "hipcc.pl" pr "hipcc.bin". As I said above, I believe the plan is to drop hipcc and rename hipcc.bin to hipcc, same with hipconfig.
I'd say it's more accurate for the hipcc package to depend on the hip package until it's moved to the hipcc source (around 5.7 ish), then I can drop the hip package and add a provides/obsoletes here.

OR I could just move everything now. Upstream has already moved everything in their development branch.

> +# Needs a %check

Good point. I think hipcc is too immature and buggy in the 5.5 branch, hence the errors.
When I tried to run hipcc.bin in the chroot with a simple compile test, it fails to find math.h, despite being installed, and the -isystem '/usr/include' being added.

I think we can put this review on hold and reconvene when 5.6 is released.

Comment 3 Jeremy Newton 2023-07-07 17:32:15 UTC
Ok I think we can proceed with this.

They've already separated HIPCC source from HIP sources, so I'm effectively bundling HIPCC source in the rocclr package for now.

Spec URL: https://mystro256.fedorapeople.org/hipcc.spec
SRPM URL: https://mystro256.fedorapeople.org/hipcc-5.6.0-3.fc39.src.rpm
Copr Build: https://copr.fedorainfracloud.org/coprs/mystro256/rocm-hip/build/6150230/

I think full deprecation will happen around ROCm 6.0, so I'll start pushing upstream to drop the .bin extension then.

Instead of a conflicts, I just handled it as a rename of the "hip" subpackage to hipcc. See rocclr.spec, I renamed "hip" to "hipcc" and copied everything over so the contents of the package should be identical other than the addition of hipcc*.bin.
Then, I'll just drop that subpackage from rocclr.spec once this is accepted. I bumped the release to higher than rocclr to make sure it happens smoothly.

Comment 4 Fedora Review Service 2023-07-07 17:38:18 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6150245
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2213372-hipcc/fedora-rawhide-x86_64/06150245-hipcc/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 5 Tom Rix 2023-07-07 21:40:45 UTC
earlier i had asked about a %check, this is because i would expect hipcc.bin to do at least something like
> touch t.hip
> hipcc.bin -c t.hip

Comment 6 Jeremy Newton 2023-07-07 21:59:16 UTC
Yeah the problem is that hipcc doesn't work by itself, it seems to require a backend like rocm-hip (libhipamd64 or libcuda).

Since cuda is closed source and hipcc is a build requirement for rocm-hip, I'd suggest moving the check over to rocm-hip so we can keep it simple.
i.e. dependency build chain goes: clang -> hipcc -> rocm-hip

Any thoughts on that?

Comment 7 Tom Rix 2023-07-08 11:11:33 UTC
hipcc.bin really doesn't run by itself, it can not handle an unknown command arg, like -v or -h.
So when I was checking it out i could not find out what version of hipcc.bin I was running.
When other folks try to do this they will likewise be confused.

The setup of clang arguments looks like it duplicates / should go into clang's Toolchain/
Then this would be a passthrough for amd and a wrapper for nvcc.
Why is the duplicated clang logic needed ?

Maybe hipcc.bin should not be in the normal PATH if it does not behave well ?
This introduces other problems so I am not sure.

It would be good if hipcc.bin were more polished / less rough.

Comment 8 Jeremy Newton 2023-07-08 22:15:32 UTC
> It would be good if hipcc.bin were more polished / less rough.

Should I exclude it for now, while I work with upstream to improve things?

I sort of want to get this going to clean up the already messy rocclr package.

Comment 9 Jeremy Newton 2024-01-10 19:38:10 UTC
So this is obsoleted by:
https://src.fedoraproject.org/rpms/rocm-compilersupport/pull-request/4

Or rather obsoleted when we move to ROCm 6.1

So I'm going to abandon this.


Note You need to log in before you can comment on or make changes to this bug.