[Bug 1668010] New: Review Request: hip- Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

Bug ID: 1668010
           Summary: Review Request: hip- Tool for porting CUDA to Portable
                    C++ Code
           Product: Fedora
           Version: rawhide
          Hardware: All
                OS: Linux
            Status: NEW
         Component: Package Review
          Severity: medium
          Priority: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]
  Target Milestone: ---
    Classification: Fedora

Spec URL:
https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...
SRPM URL: hip-1.5.18494-1.rocm2.0.0.fc30.src.rpm
Description: hip is part of the Radeon Open Compute (ROCm) ecosystem and is a
tool for porting CUDA to Portable C++ Code.
Fedora Account System Username: tstellar

[Bug 1668010] Review Request: hip- Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

Tom Stellard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]
         Depends On|                            |1545479

Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=1545479
[Bug 1545479] Review Request: hcc- Heterogeneous C++ Compiler

[Bug 1668010] Review Request: hip- Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #1 from Felix Schwarz  ---
just fyi: SRPM is likely at
https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...

[Bug 1668010] Review Request: hip- Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #2 from Tom Stellard  ---
Spec URL:
https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...
SRPM URL:
https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...

Fixed SRPM link.

[Bug 1668010] Review Request: hip- Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #3 from Tom Stellard  ---
Spec URL:
https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...
SRPM URL:
https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...

Fixed include paths so hcc headers can be found now that they are only
installed into /usr/include/hcc.

[Bug 1668010] Review Request: hip- Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #4 from Felix Schwarz  ---
unfortunately both links in comment 3 are dead (404). Can you please upload
them again/trigger a new build?

[Bug 1668010] Review Request: hip- Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #5 from Tom Stellard  ---
Spec URL:
https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...
SRPM URL:
https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...

Updated links.

[Bug 1668010] Review Request: hip- Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #6 from Felix Schwarz  ---
1. "LICENSE" file not installed (%license)

2. /usr/bin/hipconfig contains some unpatched paths:
$CUDA_PATH=$ENV{'CUDA_PATH'} // '/usr/local/cuda';
$HCC_HOME=$ENV{'HCC_HOME'} // '/opt/rocm/hcc';
$HSA_PATH=$ENV{'HSA_PATH'} // '/opt/rocm/hsa';

3. /usr/bin/hipcc - unpatched paths
 - HIP_PATH defaults to "/usr" but $HIP_INCLUDE_PATH is defined as
"$HIP_PATH/include" (-> $HIP_PATH/include/hip?).
 - from a quick glance it seems as if »$HIP_PLATFORM = "clang"« won't work as
no default path is set? (just from reading the code)
 - missing requires: hipcc requires coreutils, grep, gcc, gcc-c++ (the latter
two are only required for a CentOS 7-specific workaround)

4. /usr/lib64/libhip_hcc.so.1.5
 - executable-stack
 - E: binary-or-shlib-defines-rpath /usr/lib64/libhip_hcc.so.1.5 ['/usr/lib64']

5. hipexamine.sh, hipconvertinplace.sh non-functional
 - "hipify-clang" is no built (not built by default), both scripts rely on that
 - probably best to remove these scripts entirely (I guess there is a reason
why upstream does not build them by default)

6. In general I find that the hip package "clutters" /usr/bin/ a bit too much.
Things like 'findcode.sh' could go into "libexec" (but that might be just
personal preference). Also I'm a bit worried about the "ca" binary - that name
is extremly generic ("certificate authority"?)

7. from a quick glance it seems as if "clara.hpp" is a header-only copylib.
"bundles(clara)"?

8. license tag only lists MIT but licensecheck disagrees:
   *No copyright* GNU Lesser General Public License (v2.1 or later)
   ----------------------------------------------------------------
   HIP-roc-2.0.0/util/gedit/hip.lang

BSD 3-clause "New" or "Revised" License
   ---------------------------------------
   HIP-roc-2.0.0/samples/1_Utils/hipBusBandwidth/LICENSE.txt
   HIP-roc-2.0.0/samples/1_Utils/hipCommander/LICENSE.txt

BSL (v1.0)
   ----------
   HIP-roc-2.0.0/lpl_ca/pstreams/pstream.h

9. hipdemangleatp.sh: missing requires:
  - sed, binutils, coreutils

I have to say that I find "hip" even scarier to review with its static
libraries and all the duplicated shell/perl code...

Also I did not have time yet to actually try the hip so there may be more
issues. Anyway thanks for your work so far :-)

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

Tom Stellard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Review Request: hip- Tool   |Review Request: hip - Tool
                   |for porting CUDA to         |for porting CUDA to
                   |Portable C++ Code           |Portable C++ Code

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #7 from Tom Stellard  ---
Spec URL:
https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...
SRPM URL:
https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...

(In reply to Felix Schwarz from comment #6)
> "LICENSE" file not installed (%license)
>
> /usr/bin/hipconfig contains some unpatched paths:
>
> $CUDA_PATH=$ENV{'CUDA_PATH'} // '/usr/local/cuda';
> $HCC_HOME=$ENV{'HCC_HOME'} // '/opt/rocm/hcc';
> $HSA_PATH=$ENV{'HSA_PATH'} // '/opt/rocm/hsa';

Fixed these 2.
> /usr/bin/hipcc - unpatched paths
> HIP_PATH defaults to "/usr" but $HIP_INCLUDE_PATH is defined as
>
> "$HIP_PATH/include" (-> $HIP_PATH/include/hip?).

I think $HIP_PATH/include is correct, because the test cases I've seen use
the hip/ prefix in #include directives.
> from a quick glance it seems as if »$HIP_PLATFORM = "clang"« won't work
>
> as no default path is set? (just from reading the code)

Right, you would need to set the HIP_CLANG_PATH environment variable to make it
work.  This is by design, since I couldn't get it to work with clang-7.0.0. 
Long-term, I hope to make HIP_PLATFORM=clang the default once I can get it
working.
> missing requires: hipcc requires coreutils, grep, gcc, gcc-c++ (the
>
> latter two are only required for a CentOS 7-specific workaround)

Fixed.
> /usr/lib64/libhip_hcc.so.1.5
> executable-stack
> E: binary-or-shlib-defines-rpath /usr/lib64/libhip_hcc.so.1.5
>
> ['/usr/lib64']

Fixed.
> hipexamine.sh, hipconvertinplace.sh non-functional
> "hipify-clang" is no built (not built by default), both scripts rely on
>
> that
> probably best to remove these scripts entirely (I guess there is a reason
>
> why upstream does not build them by default)

Fixed.
> In general I find that the hip package "clutters" /usr/bin/ a bit too
>
> much. Things like 'findcode.sh' could go into "libexec" (but that might be
> just personal preference). Also I'm a bit worried about the "ca" binary -
> that name is extremly generic ("certificate authority"?)

Fixed.
> from a quick glance it seems as if "clara.hpp" is a header-only copylib.
>
> "bundles(clara)"?

I wasn't sure what to do here.  There is no clara package in Fedora, so I'm not
sure there is any reason to do bundle().
> license tag only lists MIT but licensecheck disagrees:
> *No copyright* GNU Lesser General Public License (v2.1 or later)
>
> HIP-roc-2.0.0/util/gedit/hip.lang
> BSD 3-clause "New" or "Revised" License
>
> HIP-roc-2.0.0/samples/1_Utils/hipBusBandwidth/LICENSE.txt
> HIP-roc-2.0.0/samples/1_Utils/hipCommander/LICENSE.txt
> BSL (v1.0)
>
> HIP-roc-2.0.0/lpl_ca/pstreams/pstream.h

Fixed.
> hipdemangleatp.sh: missing requires:
> sed, binutils, coreutils

Fixed.
> I have to say that I find "hip" even scarier to review with its static
> libraries and all the duplicated shell/perl code...
>
> Also I did not have time yet to actually try the hip so there may be more
> issues. Anyway thanks for your work so far :-)

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #8 from Felix Schwarz  ---
Hi Tom,

seems like nobody is willing to review this... Anyway we had a ROCm 2.4 release
recently while this package is still based on ROCm 2.0. Do you think it will be
useful to upgrade this package before a full review or is that just unnecessary
churn?

Regarding the "clara" bundling I think this is needed as per Fedora's packaging
policy ( https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling ).
See also:
https://lists.fedoraproject.org/archives/list/[e-mail]/...

I still need some time to clean up some other Fedora package (borgbackup, bug
1669083) but I plan to go through this package afterwards (and will probably
nudge a few people to either review my notes or do the formal review). I'm
really looking forward to having the ROCm stack available on Fedora easily.
(Usual disclaimer: If someone else is ready to review this please just do.)

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #9 from Tom Stellard  ---
Ok, I'll fix the bundling issue.  I think it's easiest to keep this at ROCm 2.0
until it is reviewed and then upgrade the whole stack together.

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #10 from Felix Schwarz  ---
License:        MIT and GPL and BSD and BSL

I think "GPL" was added because of "hip.lang" for gedit. "BSD" is used for two
example applications in "samples". That code is not shipped in the final
binaries as far as I can see so it should not be present in the License tag.

"BSL" probably refers to the boost software license (short name "Boost").

-> License: MIT and Boost

Also
https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuideline...
says:
> In addition, the package must contain a comment explaining the multiple licensing breakdown.

Only pstream.h uses the boost license and only "lpl" includes it so it should
be easy to document in "%files" that lpl is under "MIT AND Boot".

Requires:  bintuils

Typo: "binutils"

I tried to compile "vectoradd_hip.cpp"
(https://github.com/ROCm-Developer-Tools/HIP-Examples/blob/master/vectorAdd/v...)
with hipcc which caused this error:

$ hipcc `hipconfig --cpp_config` vectoradd_hip.cpp -o vectoradd_hip
In file included from vectoradd_hip.cpp:27:
In file included from /usr//include/hip/hip_runtime.h:56:
In file included from /usr//include/hip/hcc_detail/hip_runtime.h:96:
In file included from /usr//include/hip/hcc_detail/grid_launch_GGL.hpp:26:
In file included from
/usr//include/hip/hcc_detail/functional_grid_launch.hpp:25:
/usr//include/hip/hcc_detail/code_object_bundle.hpp:25:10: fatal error:
'hsa/hsa.h' file not found
#include <hsa/hsa.h>
         ^~~~~~~~~~~
1 error generated.

Unless I made a mistake (using F30 with a custom recompile of the rawhide
SRPMs) I think you also must depend on "rocm-runtime-devel".

My biggest gripe is how to deal with "libhip_hcc_static.a" properly
(https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-...).
A -devel package does not make much sense for hip and as far as I can see
upstream does not provide a way to prevent static linking with hipcc.

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

Robert-André Mauchin  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #11 from Robert-André Mauchin  ---
        -DHCC_HOME=/usr/ \
        -DHSA_PATH=/usr/ \

Use %{_prefix} here

- The static lib should probably not be shipped. Or if you need it, ship it in
a static subpackage.

- You need to put the include, cmake files, and unversioned library into a
devel subpackage. See
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #12 from Tom Stellard  ---
Spec URL: https://fedorapeople.org/~tstellar/hip.spec
SRPM URL:
https://fedorapeople.org/~tstellar/hip-1.5.18494-1.rocm2.0.0.fc31.src.rpm

Added bundled(clara)

(In reply to Felix Schwarz from comment #10)
> -> License: MIT and Boost

Fixed.
> Also
> https://docs.fedoraproject.org/en-US/packaging-guidelines/
> LicensingGuidelines/#_multiple_licensing_scenarios says:
> > In addition, the package must contain a comment explaining the multiple licensing breakdown.
>
> Only pstream.h uses the boost license and only "lpl" includes it so it
> should be easy to document in "%files" that lpl is under "MIT AND Boot".

Added a comment for this.
> Requires:  bintuils
>
> Typo: "binutils"

Fixed this.
> Unless I made a mistake (using F30 with a custom recompile of the rawhide
> SRPMs) I think you also must depend on "rocm-runtime-devel".

I split hip into hip-runtime and hip-runtime-devel packages like I did with
hcc, so hip no requires hip-runtime-devel which requires rocm-runtimed-devel.
> My biggest gripe is how to deal with "libhip_hcc_static.a" properly
> (https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-
> libraries). A -devel package does not make much sense for hip and as far as
> I can see upstream does not provide a way to prevent static linking with
> hipcc.

I've deleted the static library.

(In reply to Robert-André Mauchin from comment #11)
> -DHCC_HOME=/usr/ \
>    -DHSA_PATH=/usr/ \
>
> Use %{_prefix} here

This has been fixed.
> You need to put the include, cmake files, and unversioned library into a
>
> devel subpackage. See
> https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages

I've added a hip-runtime-devel sub-package and moved the mentioned files there.

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #13 from Felix Schwarz  ---
> I've deleted the static library.

$ hipcc -use-staticlib `hipconfig --cpp_config` vectoradd_hip.cpp -o
vectoradd_hip
hcc: error: no such file or directory: '/usr/lib64/libhip_hcc_static.a'

So the error message is better than expected (and probably nobody uses
hip+static linking) but I just wanted to mention that hipcc assumes the static
lib is always present.

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #14 from Felix Schwarz  ---
One more thing: If you delete the static library you also need to fix
/usr/lib64/cmake/hip/hip-targets.cmake which contains references to
"libhip_hcc_static.a". I noticed that while trying to build rocblas with your
hip package.

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #15 from Tom Stellard  ---
Spec URL:
https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...
SRPM URL:
https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...

I decided to add back the static library in its own sub-package.  I don't want
to remove the -use-staticlib option, because  it seems like it might cause us
problems in the future if fedora is missing on option supported by upstream.

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #16 from Felix Schwarz  ---
> I decided to add back the static library in its own sub-package.  I don't want to remove the -use-staticlib option, because  it seems like it might cause us problems in the future if fedora is missing on option supported by upstream.

Agreed.

However I think you forgot to add a dependency. Output when building rocblas:

-- Found PkgConfig: /usr/bin/pkg-config (found version "1.6.1") 
CMake Error at /usr/lib64/cmake/hip/hip-targets.cmake:95 (message):
  The imported target "hip::hip_hcc_static" references the file

"/usr/lib64/libhip_hcc_static.a"

but this file does not exist.  Possible reasons include:

So I think hip-runtime-devel (which owns hip-targets.cmake) should also depend
on hip-runtime-static.

@Robert-André: Do you think the current solution with the static subpackage +
hard dependency from hip-runtime-devel is ok? As far as I can tell this package
seems to be fine so I'm willing to approve that package.

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #17 from Tom Stellard  ---
Spec URL:
https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...
SRPM URL:
https://copr-be.cloud.fedoraproject.org/results/tstellar/rocm-2.0/fedora-raw...

I've added the missing dependency.

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

Robert-André Mauchin  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |POST
           Assignee|[e-mail]    |[e-mail]
              Flags|                            |fedora-review+

--- Comment #18 from Robert-André Mauchin  ---
LGTM, package approved.

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

--- Comment #19 from Gwyn Ciesla  ---
(fedscm-admin):  The Pagure repository was created at
https://src.fedoraproject.org/rpms/hip

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010

Elliott Sales de Andrade  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|POST                        |CLOSED
                 CC|                            |[e-mail]
   Fixed In Version|                            |hip-1.5.18494-1.rocm2.0.0.f
                   |                            |c31
         Resolution|---                         |CURRENTRELEASE
        Last Closed|                            |2020-08-03 00:34:49

--- Comment #20 from Elliott Sales de Andrade  ---
Please close your Review Requests when they are complete.

[Bug 1668010] Review Request: hip - Tool for porting CUDA to Portable C++ Code

https://bugzilla.redhat.com/show_bug.cgi?id=1668010
Bug 1668010 depends on bug 1545479, which changed state.

Bug 1545479 Summary: Review Request: hcc - Heterogeneous C++ Compiler
https://bugzilla.redhat.com/show_bug.cgi?id=1545479

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|POST                        |CLOSED
         Resolution|---                         |CURRENTRELEASE